-
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
Sync schema migrations #7239
Sync schema migrations #7239
Conversation
Pull Request Test Coverage Report for Build daniel.tabacaru_753
💛 - Coveralls |
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.
Since this was reviewed when it was merged into feature/sync_schema_migrations
, you can merge when any merge conflicts are resolved and the tests are green.
* Send schema version as part of BIND messages * Update comment * Changelog
* Add async shutdown of SyncSession * SyncSession::shutdown returns a Future instead of passing callbacks around * bug fix * Fix the test * A bit of redesign * Improve test * Relax check in test * Rename shutdown() to pause_async(). Hide it from the public API. * Remove unneeded header import
* Main work of sync schema migrations * misc * Code review changes
66127a6
to
44cbd35
Compare
44cbd35
to
18940aa
Compare
util::CheckedLockGuard lock(m_mutex); | ||
auto config = coordinator->get_config(); | ||
m_session = nullptr; | ||
coordinator->close(); |
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.
This is very unsafe. RealmCoordinator::close() is only safe to call if you can guarantee that no one else has a reference to the coordinator, which is not the case here. A simple example of something that'll break is if there are two async opens in progress at once.
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 think the assumption was always that for this to work only one realm coordinator/task is allowed to have the realm file opened (otherwise we cannot delete it)
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.
Well that isn't a very valid assumption.
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 see. I'll think of a different solution.
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.
it does already fail also on our ci #7325
bool set_schema = (schema_version < target_schema_version || schema_version == ObjectStore::NotVersioned || | ||
set_schema_version_on_version_decrease); |
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.
This check should just be deleted entirely as it's long obsolete. It was originally there because we used to the schema version to decide if we should create and remove indexes.
What, How & Why?
Add support for Sync Schema Migrations for flexible sync Realms. Schema versioning is introduced for synchronized realms, so a client app can open a realm at any existing version. If required, the schema is upgraded or downgraded seamlessly.
☑️ ToDos
[ ] C-API, if public C++ API changed[ ]bindgen/spec.yml
, if public C++ API changed