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

Bump the protocol version to v8 #6549

Merged
merged 9 commits into from
Apr 29, 2023
Merged

Bump the protocol version to v8 #6549

merged 9 commits into from
Apr 29, 2023

Conversation

michael-wb
Copy link
Contributor

@michael-wb michael-wb commented Apr 27, 2023

What, How & Why?

Bump the sync protocol version to v8 and remove the compile flags for protocol v8.

Fixes #6342
Fixes #6554

☑️ ToDos

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

CHANGELOG.md Outdated
@@ -3,6 +3,7 @@
### Enhancements
* <New feature description> (PR [#????](https://github.com/realm/realm-core/pull/????))
* Improve performance of rolling back write transactions after making changes. If no KVO observers are used this is now constant time rather than taking time proportional to the number of changes to be rolled back. Rollbacks with KVO observers are 10-20% faster. ([PR #6513](https://github.com/realm/realm-core/pull/6513))
* Enable PBS to FLX Migration feature that will migrate the local realm to use FLX if the server has been migrated to FLX. ([#6342](https://github.com/realm/realm-core/issues/6342))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this entry should be about the feature itself (not about enabling a feature) and also a proper link to the feature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, there isn't an issue specifically for the feature, since github issues aren't created for epics. I can update the comment to reflect the feature, rather than just enabling it..

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe @bmunkholm can work some magic

@@ -1588,6 +1588,13 @@ TEST_CASE("flx: writes work without waiting for sync", "[sync][flx][app]") {
});
}

TEST_CASE("flx: verify PBS/FLX websocket protocol number and prefix", "[sync][flx]") {
REQUIRE(8 == sync::get_current_protocol_version());
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will break when we bump the version again. You should keep the initial assertion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a check to verify that the current protocol version is not updated unexpectedly - I added comments around these to make sure they are updated when the protocol version is bumped.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, my point is that going forward the checks should still be valid unless we make other changes to the prefix and we should then assert on the new version.

@danieltabacaru
Copy link
Collaborator

Are you also adding this test? Leave a realm open across migrations and roll backs to verify bad things don’t happen

@michael-wb
Copy link
Contributor Author

Are you also adding this test? Leave a realm open across migrations and roll backs to verify bad things don’t happen

Yes - in my local changes, I have added this to the second migrate and rollback in the Test client migration and rollback with recovery test. I haven't pushed this yet, since the tests will all fail until we have the updated server. Kiro made an update this morning to fix some failures I was seeing last night.

CHANGELOG.md Outdated
@@ -2,7 +2,7 @@

### Enhancements
* <New feature description> (PR [#????](https://github.com/realm/realm-core/pull/????))
* None.
* PBS to FLX Migration for migrating a client app that uses partition based sync to use flexible sync under the hood if the server has been migrated to flexible sync. ([#6554](https://github.com/realm/realm-core/issues/6342))
Copy link
Collaborator

@danieltabacaru danieltabacaru Apr 28, 2023

Choose a reason for hiding this comment

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

the link is not correct

Copy link
Collaborator

@danieltabacaru danieltabacaru left a comment

Choose a reason for hiding this comment

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

LGTM once everything is green

Copy link
Contributor

@kmorkos kmorkos left a comment

Choose a reason for hiding this comment

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

lgtm!

@michael-wb michael-wb merged commit 901e948 into master Apr 29, 2023
@michael-wb michael-wb deleted the mwb/enable_protocol_v8 branch April 29, 2023 01:30
@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.

Implement Partition-based Sync to Flex Sync Migration Bump the protocol version to v8
4 participants