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

CloudKit - Support for limiting change set size in a CKReference-safe manner #512

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

gcox
Copy link
Contributor

@gcox gcox commented Feb 17, 2020

Resolves #212
Related: #268, #328

NOTE: This branch is based off the branch used in #510 so it could be tested

Changes

This change limits change set sizes such that all related records in a change set are kept together and that no change set can have more than a configurable max number of changes. The latter is copied from the changes in #268.

Examples

Example 1:
Records being modified: [A, B, C, D, E, F, G, H, I, J, K]
References: None
Resulting change sets: [[A, B, C, D], [E, F, G, H], [I, J, K]]

Example 2:
Records being modified: [A, B, C, D, E, F, G, H, I, J, K]
References: D -> F
Limit = 4
Resulting change sets: [[A, B, C], [D, E, F, G], [H, I, J, K]

Concerns/questions

If a parent and child record are modified, are they always sequentially close to each other? IOW, if the max change limit is 400, and 500 objects are imported, including records Foo and Bar, and Foo references Bar, is it possible that enumerating the dirty records will find record Foo at index 0 and record Bar at index 500? If so, this solution doesn't cover that scenario.

When processing dirty records, does the order of the records in the resulting change sets matter? If not, we could refactor this so that when generating the change sets shift referenced records so they are next to their parents to avoid the above edge case.

TODO

  • Tweak the implementation so that we're not forcing child records to be in the same change set with their parent records unless those child records are new
  • Add tests once a general solution is agreed on. Whatever we end up with is going to add too much complexity not to be tested.

gcox and others added 9 commits February 17, 2020 06:01
* This is so users can just add their own Signing.xcconfig file with their dev team and a bundle id they're allowed to create. This keeps team info out of the project file. The Signing.xcconfig file is now added to the .gitignore file.
Adds a couple of TODOs:
1. objectPolicy/metadataPolicy issue: yapstudios#502
2. YDBLogContext issue: yapstudios#509
The change adds an initial enumeration of the dirty records to find build a map of all the parent references that have references that are also changing.
The existing enumeration was modified to ensure change sets are as large as possible given the configurable limit AND keep parent/child references in the same change set.
* Fixes issue that would leave the `isResolvingReferences` flag set to YES
@gcox
Copy link
Contributor Author

gcox commented Feb 17, 2020

@robbiehanson Your feedback on the 'Concerns/Questions' section would be appreciated. Then I can finish this up with some tests.

@robbiehanson
Copy link
Contributor

There might be a bug here:

// Build a map of dirty record CKRecordIDs to all of their associated CKReferences'
NSMutableDictionary<CKRecordID *, NSMutableArray<CKRecordID *> *> *referenceMap = NSMutableDictionary.new;

[parentConnection->dirtyRecordTableInfoDict
   enumerateKeysAndObjectsUsingBlock:^(NSString * _Nonnull key,
                                       YDBCKDirtyRecordTableInfo * _Nonnull obj,
                                       BOOL * _Nonnull stop) {
if (obj.dirty_record.parent) {
    NSMutableArray<CKRecordID *> *parentReferences =
      (referenceMap[obj.dirty_record.parent.recordID] ?: NSMutableArray.new);

    [parentReferences addObject:obj.dirty_record.recordID];
    referenceMap[obj.dirty_record.recordID] = parentReferences;
  }
}];

For referenceMap is the key supposed to be obj.dirty_record.recordID or obj.dirty_record.parent.recordID?

@gcox
Copy link
Contributor Author

gcox commented Feb 17, 2020

@robbiehanson oh yes that’s a bug, should be obj.dirty_record.parent.recordID

@robbiehanson
Copy link
Contributor

robbiehanson commented Feb 17, 2020

If a parent and child record are modified, are they always sequentially close to each other? IOW, if the max change limit is 400, and 500 objects are imported, including records Foo and Bar, and Foo references Bar, is it possible that enumerating the dirty records will find record Foo at index 0 and record Bar at index 500?

I think this is possible because we're currently storing changes in a dictionary (parentConnection->dirtyRecordTableInfoDict). And a NSDictionary doesn't give us any guarantees about order when we enumerate it.

When processing dirty records, does the order of the records in the resulting change sets matter?

Great question. I would say the order doesn't matter, but that it would be preferable to maintain the order when possible. For example:

Records being modified: [A, B, C, D, E, F, G, H, I, J, K]
References: A -> K
Limit = 4
Resulting change sets: [[A, K, B, C], [D, E, F, G], [H, I, J]

So it sounds like parentConnection->dirtyRecordTableInfoDict is not going to give us everything we need. We also want to store the order of the records. So either we convert the dictionary to an array, or store an array alongside the dictionary to record the order?

@gcox
Copy link
Contributor Author

gcox commented Feb 17, 2020

@robbiehanson Thanks for the input. I’ll take a look at both options.

@gcox
Copy link
Contributor Author

gcox commented Feb 26, 2020

@robbiehanson I believe the latest changes will produce this:

Records being modified: [A, B, C, D, E, F, G, H, I, J, K]
References: A -> K
Limit = 4
Resulting change sets: [[A, K, B, C], [D, E, F, G], [H, I, J]

Gotta figure out a reasonable way to test this area though

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.

YapDatabaseCloudKit with a large amount of data.
3 participants