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

Upload data and migrate schema #6944

Conversation

danieltabacaru
Copy link
Collaborator

@danieltabacaru danieltabacaru commented Sep 4, 2023

What, How & Why?

This is the main body of work for sync schema migrations.

A schema migrations is performed whenever the client receives schema_version_changed error (236) from the server. In such cases, the synchronization session is left active so any unsynced changes are uploaded to the server before migrating the schema. The realm file is then deleted and reopened at the new schema version. Data is bootstrapped at the new schema version.

These are the steps the client and server take in case of a schema migration:

  • Client BINDs with the new schema version
  • Client IDENTs
  • Server detects a difference in the previous file version and new version, sends an error schema version changed
  • Client uploads its local data at the old schema version
  • Client waits for acknowledgement of upload
  • Client deletes the realm, reopens it, and rebootstraps at new schema version

Note: Schema migrations can only be performed when a realm is opened asynchronously.

Fixes #6842.

Integration tests will be added in a separate PR.

☑️ ToDos

  • [ ] 📝 Changelog update
  • [ ] 🚦 Tests (or not relevant)
  • [ ] C-API, if public C++ API changed.

Copy link
Contributor

@michael-wb michael-wb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good - just a few comments.

Comment on lines +66 to +70
if (auto self = weak_self.lock()) {
util::CheckedLockGuard lock(m_mutex);
if (!m_session)
return;
m_sync_schema_migration_required = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there ever a case where weak_self could be locked but the this pointer is not valid? I can't think of any.

Copy link
Collaborator Author

@danieltabacaru danieltabacaru Sep 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's really possible either

Comment on lines +335 to +338
#if REALM_ENABLE_SYNC
if (m_config.sync_config)
return;
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this check for? Is it to make sure it is not a sync'ed realm?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah. didn't want to break the behaviour for local realms.

return;
}

std::shared_ptr<AsyncOpenTask> self(shared_from_this());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment that you are extending the lifetime of this object until the bootstrap is complete?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a comment. we've already been doing that with the initial subscriptions callback.

std::function<void()> callback;
{
util::CheckedLockGuard l(m_state_mutex);
callback = m_sync_schema_migration_callback;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you "move" or clear the callback when you grab it to call it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, let's do that.

@danieltabacaru danieltabacaru merged commit 1ebbeff into feature/sync_schema_migrations Sep 11, 2023
23 of 26 checks passed
@danieltabacaru danieltabacaru deleted the dt/upload_data_and_migrate_schema branch September 11, 2023 10:25
danieltabacaru added a commit that referenced this pull request Nov 14, 2023
* Main work of sync schema migrations

* misc

* Code review changes
danieltabacaru added a commit that referenced this pull request Dec 14, 2023
* Main work of sync schema migrations

* misc

* Code review changes
danieltabacaru added a commit that referenced this pull request Jan 8, 2024
* Main work of sync schema migrations

* misc

* Code review changes
danieltabacaru added a commit that referenced this pull request Jan 15, 2024
* Main work of sync schema migrations

* misc

* Code review changes
danieltabacaru added a commit that referenced this pull request Jan 22, 2024
* Add schema version to flexible sync client BIND message (#6863)

* Send schema version as part of BIND messages

* Add pause_async to SyncSession (#6845)

* Add async shutdown of SyncSession

* SyncSession::shutdown returns a Future instead of passing callbacks around

* Rename shutdown() to pause_async(). Hide it from the public API.

* Remove unneeded header import

* Upload data and migrate schema (#6944)

* Handle client resets during schema migrations + integration tests (#7106)

* Add option to use draft deployments when creating a schema for baas tests

* Track sync schema migration between sessions + minor refactoring

* Integration tests

* Bump sync protocol version to 11

* Update baas commit for evergreen tests

* Allow resetting schema version to zero

* Improve changelog
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants