-
Notifications
You must be signed in to change notification settings - Fork 169
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
Assorted sync metadata storage refactoring #7329
Conversation
6d879f9
to
ded9c66
Compare
ded9c66
to
2be351d
Compare
Pull Request Test Coverage Report for Build thomas.goyne_158
💛 - Coveralls |
2be351d
to
241668b
Compare
try { | ||
SyncUserMetadata data(m_user_schema, realm, user); | ||
file_manager.remove_user_realms(data.identity(), data.realm_file_paths()); | ||
realm->begin_transaction(); |
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.
should we delete all users in a single transaction? The downside would be that in case of an exception (other than FileAccessError), no user is removed.
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.
I am currently undecided on what level of batching is correct. This PR is matching the existing behavior and it's just much more obvious that we're performing write transactions in a loop due to the code movement.
If I understand these changes correctly, we got rid of |
The AppMetadata table won't be deleted from existing files (that requires a schema version bump and a migration), but it won't be created in newly initialized metadata realms. |
Yes, that's what I meant. |
This is pulling some of the incidental changes out of #7300 since that PR is growing far too large. None of this has meaningful functional changes on its own and is mostly just pushing code around.
One of the upcoming changes is making SyncUsers lazily loaded and reloaded when the data in the metadata store changes, which benefits from it being relatively cheap to validate that the stored user is valid. Changing RealmJWT to operate on a string view had cascading effects on base64 and exjson parsing, which I also updated to use Span rather than a pointer+size while I was touching it.
Checking if the data in the metadata realm is valid was done incompletely in three different places, and now is just in SyncMetadataManager. File action handling is also there now as that's going to be required for it to be multiprocess safe. Most of SyncMetadataManager's API is now unused outside of tests, but is left in place for now as removing it requires rewriting the tests (which is done in #7300).
CheckedMutex::lock() is
const
, so it doesn't have to be declared asmutable
.