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

Add schema version to flexible sync client BIND message #6863

Merged

Conversation

danieltabacaru
Copy link
Collaborator

@danieltabacaru danieltabacaru commented Aug 4, 2023

What, How & Why?

For the Sync Schema Migration project the sync client sends the schema version the realm is opened with as part of the BIND messages. The schema version is added to the JSON data section of the FLX BIND message.

Fixes #6840.

☑️ ToDos

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

@cla-bot cla-bot bot added the cla: yes label Aug 4, 2023
@danieltabacaru danieltabacaru marked this pull request as ready for review August 4, 2023 20:12
@danieltabacaru danieltabacaru changed the title Send schema version as part of BIND messages Add schema version to flexible sync client BIND message Aug 7, 2023
@@ -1861,6 +1859,7 @@ void Session::send_bind_message()
if (auto migrated_partition = get_migration_store()->get_migrated_partition()) {
bind_json_data["migratedPartition"] = *migrated_partition;
}
bind_json_data["schema_version"] = get_schema_version();
Copy link
Contributor

Choose a reason for hiding this comment

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

I am assuming the server will treat a -1 schema version the same as if the value was not provided.
Should we exclude the schema_version value if it is "not versioned"?
or maybe exclude it for now and always include it once we have tested core against the server schema versions feature?
If there are changes to core required to support the new feature, it could be used to advertise to the server that we support the feature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are client changes required for this feature. One way to advertise the client supports the feature is by bumping the sync protocol version (we're still debating if it's really needed). My thinking was that once an app is upgraded to support the future, the client will always report a schema version even if there is none.

We can always stop reporting the schema version in some cases if we deem it necessary (this is all going to a feature branch).

Copy link
Contributor

Choose a reason for hiding this comment

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

With the bind json data, I don't think you need to bump the proto version - hopefully, the presence of the value should be enough to indicate whether or not the client supports schema migrations (which was my concern if this was being merged to master). Although, this only works for FLX, and the proto version would need to be bumped if it also has to work for PBS.

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, that's the idea. And that's why I think it would be a bit odd to sometimes not report it (that should only happen if the app is downgraded). But this may change once we start integration testing.

@danieltabacaru danieltabacaru changed the base branch from master to feature/sync_schema_migrations August 7, 2023 14:05
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.

LGTM based on the conversation and the use of the feature branch.

Comment on lines 354 to 355
/// Schema version
uint64_t schema_version = -1; // = ObjectStore::NotVersioned
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend updating the comment to indicate it's for FLX only, or update the value to flx_schema_version since this feature is only for FLX sync.

@danieltabacaru danieltabacaru merged commit 5deea82 into feature/sync_schema_migrations Aug 7, 2023
@danieltabacaru danieltabacaru deleted the dt/send_schema_version_in_bind branch August 7, 2023 21:59
danieltabacaru added a commit that referenced this pull request Nov 14, 2023
* Send schema version as part of BIND messages

* Update comment

* Changelog
danieltabacaru added a commit that referenced this pull request Dec 14, 2023
* Send schema version as part of BIND messages

* Update comment

* Changelog
danieltabacaru added a commit that referenced this pull request Jan 8, 2024
* Send schema version as part of BIND messages

* Update comment

* Changelog
danieltabacaru added a commit that referenced this pull request Jan 15, 2024
* Send schema version as part of BIND messages

* Update comment

* Changelog
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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send schemaVersion in BIND message
2 participants