-
Notifications
You must be signed in to change notification settings - Fork 152
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
Make sync session multiplexing on-by-default #6557
Conversation
fi | ||
|
||
if [ -n "${enable_sync_multiplexing|}" ]; then | ||
set_cmake_var realm_vars REALM_ENABLE_SYNC_MULTIPLEXING BOOL On | ||
if [ -n "${enable_llvm_coverage|}" ]; then |
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.
Not sure why these switched places? I guess a weird artifact of a git merge?
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.
LGTM - just one tiny change
evergreen/config.yml
Outdated
if [ -n "${disable_sync_multiplexing|}" ]; then | ||
set_cmake_var realm_vars REALM_DISABLE_SYNC_MULTIPLEXING BOOL On |
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 you need to update this to match the REALM_SYNC_MULTIPLEXING
cmake flag
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.
Fixed.
SECTION("allowed") { | ||
sync_multiplexing_allowed = true; | ||
} | ||
SECTION("not allowed") { | ||
sync_multiplexing_allowed = false; | ||
} |
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 cool how we can just do this to minimizing the code dups
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 actually not great as it results in Catch not reporting which mode failed if the test does fail, as the failure won't happen inside the section. bool sync_multiplexing_allowed = GENERATE(false, true);
fixes that and is a bit shorter as a bonus.
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.
Neat, I didn't know about GENERATE()
. That's a good tip.
What, How & Why?
This updates the sync client's default configuration so that session multi-plexing is on-by-default. The test suite where session multiplexing was enabled now tests the sync client with multiplexing disabled. The
enable_session_multiplexing()
method onSyncManager
is nowset_session_multiplexing()
which lets you explicitly set it to enabled or disabled regardless of how realm was compiled. I also added some object-store level integration tests that test that the function works.☑️ ToDos