Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

send parents first and update foreign keys with sendAll #95

Closed
wants to merge 5 commits into from

Conversation

tomaszn
Copy link
Contributor

@tomaszn tomaszn commented Oct 22, 2017

A new function calculates the proper sync order for models. It is used in outbox.sendAll.

@tomaszn tomaszn force-pushed the send_parents_first branch 3 times, most recently from 8c41e2a to 1cd9e4a Compare October 22, 2017 22:07
@tomaszn tomaszn changed the title send parents first with sendAll (fixes #94) send parents first with sendAll Nov 4, 2017
@tomaszn
Copy link
Contributor Author

tomaszn commented Nov 4, 2017

This should fix #94

@tomaszn tomaszn changed the title send parents first with sendAll send parents first and update foreign keys with sendAll Nov 4, 2017
@sheppard
Copy link
Member

Ok, I finally had a chance to look at this. I started by implementing a test case which appears to be working (see #94), other than an issue that came up while testing (#102). So, my first suggestion would be to extend the test case and see if you can get it to fail.

Even if it is working, there is certainly room for improvement in how this is implemented. There are two main considerations here, which we can address individually.

  1. First, there is the question of how to determine the hierarchy between different models. The current implementation searches the submitted form data for the pattern /^outbox-(\d+)$/ and creates a parents property if it is found (a968a75). This PR would change that approach to rely on wq:ForeignKey links in the actual model configuration.

    • Advantages of the current approach:
      • does not require wq/outbox to be aware of the model configuration
      • only creates a sync hierarchy when the parent record lacks an existing id
    • Advantages of this PR:
      • uses formal schema rather than regular expression hack
      • allows the use of "outbox-[number]" as a plain string value in non-ForeignKey fields
  2. Second, there is the question of how to ensure that the records are sent to the server in the correct (hierarchical) order. The current approach is to lock any outbox records with a parents property, thus ensuring that only the highest-level models are synced first. As each parent record is synced, the parents property for each child record is updated and then removed when complete. The unlocked child records are then synced on the next attempt.
     
    Note that in a968a75, only one level would be synced at a time. So, if there was a three-level hierarchy, it would take three successive calls to sendAll() until the last record was sent. I fixed that in 2bb1c31 to make sendItems() attempt to recursively call itself until everything is synced.
     
    This PR uses an iterative approach instead: the order is precomputed and then executed.

@tomaszn
Copy link
Contributor Author

tomaszn commented Jan 19, 2018

I think that it's good to keep in mind that developing the batch service would be the next step, and it can be done in different ways too. Besides improving performance, especially in comparison to my request-by-request way in rural areas (it takes literally hours), and also solving #102, it could enable (even circular) dependencies between items. When no attachments are synced, probably one big request would be ideal, but in case of bigger attachments the app would need to revert to the old behaviour, so sync order or hierarchical sync still need to be implemented.

I though about writing a test to ensure that any implementation doesn't attempt sending dependent records after the parent failed. But in the real project I need to track these locked records too, and currently I am able to dig the web server logs to recover them. Batch service wouldn't break this.

@sheppard sheppard added this to the 1.2 - Redux Integration milestone Apr 17, 2019
@sheppard
Copy link
Member

sheppard commented Aug 8, 2019

I believe integrating Redux Offline addressed the concerns here (and in #102). In particular, Redux Offline uses a lock, an in-memory store, and (by default) only sends one record at a time. The one case where foreign key ordering is still important is in the new batch mode - see #110.

@sheppard sheppard closed this Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants