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

RCORE-2129 Do not send empty upload messages if there are no downloads to ack #7859

Merged
merged 7 commits into from
Jul 19, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

### Fixed
* When a public name is defined on a property, calling `realm::Results::sort()` or `realm::Results::distinct()` with the internal name could throw an error like `Cannot sort on key path 'NAME': property 'PersonObject.NAME' does not exist`. ([realm/realm-js#6779](https://github.com/realm/realm-js/issues/6779), since v12.12.0)
* Fixed sending empty UPLOAD messages when there is no download to acknowledge ([#2129](https://jira.mongodb.org/browse/RCORE-2129), since v14.8.0).
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a correctness issue or a performance improvement? I don't think it is clear to someone just reading this line what the impact was.

Copy link
Collaborator Author

@danieltabacaru danieltabacaru Jul 18, 2024

Choose a reason for hiding this comment

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

It's a performance improvement. It saves roundtrips to the server. But it's a bug fix since we introduced it in 14.8.0. I'll reword it.


### Breaking changes
* None.
Expand Down
14 changes: 9 additions & 5 deletions src/realm/sync/noinst/client_impl_base.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2028,17 +2028,21 @@ void Session::send_upload_message()
target_upload_version = m_pending_flx_sub_set->snapshot_version;
}

bool server_version_to_ack =
m_upload_progress.last_integrated_server_version < m_download_progress.server_version;

std::vector<UploadChangeset> uploadable_changesets;
version_type locked_server_version = 0;
get_history().find_uploadable_changesets(m_upload_progress, target_upload_version, uploadable_changesets,
locked_server_version); // Throws

if (uploadable_changesets.empty()) {
// Nothing more to upload right now
// If we need to limit upload up to some version other than the last client version available and there are no
// changes to upload, then there is no need to send an empty message.
if (m_pending_flx_sub_set) {
logger.debug("Empty UPLOAD was skipped (progress_client_version=%1, progress_server_version=%2)",
// Nothing more to upload right now if:
// 1. We need to limit upload up to some version other than the last client version
// available and there are no changes to upload
// 2. There are no changes to upload and no server version(s) to acknowledge
if (m_pending_flx_sub_set || !server_version_to_ack) {
logger.trace("Empty UPLOAD was skipped (progress_client_version=%1, progress_server_version=%2)",
m_upload_progress.client_version, m_upload_progress.last_integrated_server_version);
// Other messages may be waiting to be sent
return enlist_to_send(); // Throws
Expand Down
38 changes: 38 additions & 0 deletions test/object-store/sync/flx_sync.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5023,6 +5023,44 @@ TEST_CASE("flx: nested collections in mixed", "[sync][flx][baas]") {
CHECK(nested_list.get_any(1) == "foo");
}

TEST_CASE("flx: no upload during bootstraps", "[sync][flx][bootstrap][baas]") {
FLXSyncTestHarness harness("flx_bootstrap_no_upload", {g_large_array_schema, {"queryable_int_field"}});
fill_large_array_schema(harness);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to use the large array schema here with tons of data to upload/download?

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 think in a single download message bootstrap the data is applied immediately before the client gets the chance to send an upload (and after the bootstrap is done there is a server version to ack so the upload is legit). but I can check.

bool should_send_upload = true;

SyncTestFile config(harness.app()->current_user(), harness.schema(), SyncConfig::FLXSyncEnabled{});
Copy link
Contributor

Choose a reason for hiding this comment

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

you can do auto config = harness.make_test_file() as a shortcut here

config.sync_config->on_sync_client_event_hook = [&](std::weak_ptr<SyncSession> weak_session,
const SyncClientHookData& data) {
if (data.query_version == 0) {
return SyncClientHookAction::NoAction;
}
auto session = weak_session.lock();
Copy link
Contributor

Choose a reason for hiding this comment

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

do we actually use the session here anywhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no. i'll remove it.

if (!session) {
return SyncClientHookAction::NoAction;
}
// Check no upload messages are sent during bootstrap.
if (data.event == SyncClientHookEvent::BootstrapMessageProcessed) {
should_send_upload = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to check that we actually hit these cases in the test somehow?

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 could add a state machine (considered that actually)

}
else if (data.event == SyncClientHookEvent::DownloadMessageIntegrated &&
data.batch_state == sync::DownloadBatchState::LastInBatch) {
should_send_upload = true;
}
else if (data.event == SyncClientHookEvent::UploadMessageSent) {
CHECK(should_send_upload);
}

return SyncClientHookAction::NoAction;
};

auto realm = Realm::get_shared_realm(config);
auto table = realm->read_group().get_table("class_TopLevel");
auto new_subs = realm->get_latest_subscription_set().make_mutable_copy();
new_subs.insert_or_assign(Query(table));
auto subs = new_subs.commit();
subs.get_state_change_notification(sync::SubscriptionSet::State::Complete).get();
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add something here where we do a write in the DB that can't result in an upload - like say writing to a table not prefixed by class_ and make sure it doesn't send any uploads?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

great suggestion.

}

} // namespace realm::app

#endif // REALM_ENABLE_AUTH_TESTS
Loading