-
Notifications
You must be signed in to change notification settings - Fork 167
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
Simplify notifier registration and collection replication #6513
Conversation
5a3dc27
to
e9240be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice to see some simplification of the notifier code!
@@ -48,9 +48,8 @@ struct ListChangeInfo { | |||
}; | |||
|
|||
struct TransactionChangeInfo { | |||
std::vector<ListChangeInfo> lists; | |||
std::vector<ListChangeInfo> collections; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider changing the type to be called "CollectionChangeInfo"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I took a look at all of the places related to this which were using the word "list" and changed a bunch to "collection". The two remaining exceptions are ListNotifier, which runs into the problem that we already have something named CollectionNotifier (and more broadly the problem that SDKs consider Results a collection but core kinda doesn't), and the KVO stuff which on the core side could work with all collections, but is NSArray-specific in obj-c.
fd41617
to
f3248c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the naming changes, that'll be much nicer for readability when we come to visit this code next.
f3248c2
to
d7aedd0
Compare
Notifications treat all collection types identically, so non-sync replication doesn't need to emit different instructions for each collection type. Parsing the old instructions is still supported to avoid having to bump the lockfile version.
When the notification code was originally written, opening a new SharedGroup was a very expensive operation and it was important to minimize the number of them used. This lead to some complicated logic to handle multiple source versions for notifiers where the single SharedGroup was incrementally advanced between the versions. This is now all obsolete; newly registered notifiers hold their own Transaction and can just be advanced separately.
This was used by the global notifier and has been unused ever since that was deleted.
We don't actually need TransactReverser for anything other than KVO these days, and the KVO parser can trivially do the required reversing itself. Sending schema change notifications from a rolled back write transaction was actually just incorrect to begin with, but was harmless other than being suboptimal.
…e actually used now
d7aedd0
to
78d4ffd
Compare
Notifications treat all collection types identically, so non-sync replication doesn't need to emit different instructions for each collection type. This eliminates some duplicated code. Parsing the old instructions is still supported to avoid having to bump the lockfile version.
When the notification code was originally written, opening a new SharedGroup was a very expensive operation and it was important to minimize the number of them used. This lead to some complicated logic to handle multiple source versions for notifiers where the single SharedGroup was incrementally advanced between the versions. This is now all obsolete; newly registered notifiers hold their own Transaction and can just be advanced separately.
TransactionChangeInfo::track_all was part of the global notifier and has been unused ever since that was deleted.
We don't actually need TransactReverser for anything other than KVO these days, and the KVO parser can trivially do the required reversing itself. Sending schema change notifications from a rolled back write transaction was actually just incorrect to begin with, but was harmless other than being suboptimal.