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

danieltabacaru · 2024-07-02T08:55:53Z

What, How & Why?

We were sending unnecessary uploads to the server when there was no data to synchronize and no downloads to acknowledge.

Fixes #7711.

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)
~~[ ] C-API, if public C++ API changed~~
~~[ ] bindgen/spec.yml, if public C++ API changed~~

coveralls-official · 2024-07-02T09:50:59Z

Pull Request Test Coverage Report for Build daniel.tabacaru_852

Details

4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
42 unchanged lines in 9 files lost coverage.
Overall coverage increased (+0.003%) to 90.977%

Files with Coverage Reduction	New Missed Lines	%
src/realm/query_engine.hpp	1	93.94%
src/realm/sort_descriptor.cpp	1	94.06%
src/realm/sync/noinst/client_impl_base.cpp	1	82.41%
src/realm/cluster.cpp	2	75.85%
test/test_lang_bind_helper.cpp	2	93.2%
src/realm/table.cpp	3	90.42%
test/fuzz_group.cpp	9	42.26%
src/realm/sync/noinst/server/server.cpp	10	73.96%
src/realm/bplustree.cpp	13	71.41%

Totals
Change from base Build 2466:	0.003%
Covered Lines:	215137
Relevant Lines:	236473

💛 - Coveralls

coveralls-official · 2024-07-10T10:16:22Z

Pull Request Test Coverage Report for Build daniel.tabacaru_872

Details

47 of 47 (100.0%) changed or added relevant lines in 2 files are covered.
108 unchanged lines in 10 files lost coverage.
Overall coverage decreased (-0.03%) to 90.969%

Files with Coverage Reduction	New Missed Lines	%
src/realm/index_string.hpp	1	93.48%
src/realm/list.cpp	1	87.52%
src/realm/util/serializer.cpp	1	90.43%
test/test_index_string.cpp	1	93.48%
src/realm/sync/network/http.hpp	2	82.27%
src/realm/sync/client.cpp	3	90.98%
src/realm/unicode.cpp	3	83.83%
test/util/dump_changesets.cpp	8	42.86%
src/realm/sync/noinst/server/server.cpp	29	73.16%
test/fuzz_group.cpp	59	42.11%

Totals
Change from base Build 2483:	-0.03%
Covered Lines:	215240
Relevant Lines:	236607

💛 - Coveralls

jbreams · 2024-07-11T19:50:52Z

test/object-store/sync/flx_sync.cpp

+        }
+        // Check no upload messages are sent during bootstrap.
+        if (data.event == SyncClientHookEvent::BootstrapMessageProcessed) {
+            should_send_upload = false;


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

I could add a state machine (considered that actually)

jbreams · 2024-07-11T19:52:04Z

test/object-store/sync/flx_sync.cpp

+        if (data.query_version == 0) {
+            return SyncClientHookAction::NoAction;
+        }
+        auto session = weak_session.lock();


do we actually use the session here anywhere?

no. i'll remove it.

jbreams · 2024-07-11T19:53:35Z

test/object-store/sync/flx_sync.cpp

+    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();


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?

great suggestion.

jbreams · 2024-07-11T19:54:30Z

test/object-store/sync/flx_sync.cpp

@@ -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);


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

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.

jbreams · 2024-07-11T19:55:05Z

test/object-store/sync/flx_sync.cpp

+    fill_large_array_schema(harness);
+    bool should_send_upload = true;
+
+    SyncTestFile config(harness.app()->current_user(), harness.schema(), SyncConfig::FLXSyncEnabled{});


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

michael-wb · 2024-07-17T17:11:28Z

test/object-store/sync/flx_sync.cpp

+        // Check no upload messages are sent during bootstrap.
+        if (data.event == SyncClientHookEvent::BootstrapMessageProcessed) {
+            CHECK((state.get() == TestState::Start || state.get() == TestState::BootstrapInProgress));
+            state.transition_to(TestState::BootstrapInProgress);
+        }
+        else if (data.event == SyncClientHookEvent::DownloadMessageIntegrated &&
+                 data.batch_state == sync::DownloadBatchState::LastInBatch) {
+            CHECK(state.get() == TestState::BootstrapInProgress);
+            state.transition_to(TestState::BootstrapProcessed);
+        }
+        else if (data.event == SyncClientHookEvent::UploadMessageSent) {
+            // Uploads are allowed before a bootstrap starts.
+            if (state.get() == TestState::Start) {
+                return SyncClientHookAction::NoAction;
+            }
+            CHECK(state.get() == TestState::BootstrapProcessed);
+            state.transition_to(TestState::BootstrapAck);
+        }


It might be more efficient to add this part under a lambda provided to state.transition_with() since the mutex will only be locked once (and held) instead of every time for the state.get() and state.transition_to() calls.

michael-wb · 2024-07-17T19:12:32Z

It's not related to your changes, but would also you mind updating this debug statement (one that I added) so it is consistent with the other UPLOAD messages (perhaps remove the : upload progress part):
https://github.com/realm/realm-core/blob/master/src/realm/sync/noinst/client_impl_base.cpp#L2057-L2058

danieltabacaru · 2024-07-18T09:02:03Z

It's not related to your changes, but would also you mind updating this debug statement (one that I added) so it is consistent with the other UPLOAD messages (perhaps remove the : upload progress part): https://github.com/realm/realm-core/blob/master/src/realm/sync/noinst/client_impl_base.cpp#L2057-L2058

Don't we need to call enlist_to_send() after that statement? I think we got a bit lucky because we send queries before uploads, otherwise the client would get stuck.

michael-wb · 2024-07-18T14:00:42Z

It's not related to your changes, but would also you mind updating this debug statement (one that I added) so it is consistent with the other UPLOAD messages (perhaps remove the : upload progress part): master/src/realm/sync/noinst/client_impl_base.cpp#L2057-L2058

Don't we need to call enlist_to_send() after that statement? I think we got a bit lucky because we send queries before uploads, otherwise the client would get stuck.

I think you're right - there's another place where we return early and it calls enlist_to_send() - Good catch!
https://github.com/realm/realm-core/blob/master/src/realm/sync/noinst/client_impl_base.cpp#L2044

michael-wb

LGTM - thanks for making the update

ironage · 2024-07-18T20:42:15Z

test/object-store/sync/flx_sync.cpp

+    realm->begin_transaction();
+    realm->commit_transaction();
+    wait_for_upload(*realm);


How would this fail without your changes, would it hang?

it would send an upload message. But a recent change made wait_for_upload return immediately if all remaining changesets to upload are empty, so it may not give the client the chance to send the upload (and hence not fail without the change). wait_for_download() makes more sense because after MARK is sent, then an upload would be sent.

ironage · 2024-07-18T20:43:35Z

CHANGELOG.md

@@ -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).


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.

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.

cla-bot bot added the cla: yes label Jul 2, 2024

github-actions bot assigned danieltabacaru Jul 2, 2024

danieltabacaru force-pushed the dt/fix-unnecessary-uploads branch from e8eae22 to e4c353b Compare July 8, 2024 08:49

danieltabacaru marked this pull request as ready for review July 10, 2024 06:49

danieltabacaru requested a review from jbreams July 10, 2024 06:50

danieltabacaru added 2 commits July 10, 2024 10:38

Do not send empty upload message if there is no download message to ack

b71ce67

Add integration test

f6a363d

danieltabacaru force-pushed the dt/fix-unnecessary-uploads branch from 153332b to f6a363d Compare July 10, 2024 08:38

Merge branch 'master' into dt/fix-unnecessary-uploads

3732aef

jbreams reviewed Jul 11, 2024

View reviewed changes

Code review feedback

448bd53

danieltabacaru requested review from jbreams and michael-wb July 12, 2024 14:03

michael-wb reviewed Jul 17, 2024

View reviewed changes

Code review changes

d344642

Check if other messages need to be sent when uploads are not allowed

d048425

danieltabacaru requested a review from ironage July 18, 2024 18:32

michael-wb approved these changes Jul 18, 2024

View reviewed changes

ironage reviewed Jul 18, 2024

View reviewed changes

Code review changes

5224176

ironage approved these changes Jul 18, 2024

View reviewed changes

danieltabacaru merged commit a59aeae into master Jul 19, 2024
39 of 41 checks passed

danieltabacaru deleted the dt/fix-unnecessary-uploads branch July 19, 2024 06:31

danieltabacaru mentioned this pull request Jul 19, 2024

Fix bad merge #7900

Merged

1 task

github-actions bot mentioned this pull request Jul 19, 2024

Prepare for 14.11.0 #7901

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

danieltabacaru commented Jul 2, 2024 •

edited

Loading

coveralls-official bot commented Jul 2, 2024 •

edited

Loading

coveralls-official bot commented Jul 10, 2024 •

edited

Loading

jbreams Jul 11, 2024

danieltabacaru Jul 11, 2024

jbreams Jul 11, 2024

danieltabacaru Jul 11, 2024

jbreams Jul 11, 2024

danieltabacaru Jul 11, 2024

jbreams Jul 11, 2024

danieltabacaru Jul 11, 2024

jbreams Jul 11, 2024

michael-wb Jul 17, 2024

michael-wb commented Jul 17, 2024

danieltabacaru commented Jul 18, 2024

michael-wb commented Jul 18, 2024

michael-wb left a comment

ironage Jul 18, 2024

danieltabacaru Jul 18, 2024

ironage Jul 18, 2024

danieltabacaru Jul 18, 2024 •

edited

Loading

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

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

Conversation

danieltabacaru commented Jul 2, 2024 • edited Loading

What, How & Why?

☑️ ToDos

coveralls-official bot commented Jul 2, 2024 • edited Loading

Pull Request Test Coverage Report for Build daniel.tabacaru_852

Details

💛 - Coveralls

coveralls-official bot commented Jul 10, 2024 • edited Loading

Pull Request Test Coverage Report for Build daniel.tabacaru_872

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-wb commented Jul 17, 2024

danieltabacaru commented Jul 18, 2024

michael-wb commented Jul 18, 2024

michael-wb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieltabacaru Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

danieltabacaru commented Jul 2, 2024 •

edited

Loading

coveralls-official bot commented Jul 2, 2024 •

edited

Loading

coveralls-official bot commented Jul 10, 2024 •

edited

Loading

danieltabacaru Jul 18, 2024 •

edited

Loading