RCORE-2099 Restore progress notifier behavior when sync session is already caught up #7681

jbreams · 2024-05-07T18:18:01Z

What, How & Why?

This is split up into a few commits.

Contains the functional changes - basically reverts the object-store behavior of progress notifications back to before the latest changes and reverts the tests back to before the latest changes. Not sure the best way to review this in github, but the tests should look like they did in 255cb33, only with the query_version threaded through for FLX and checks progress estimates for PBS are what we expect them to be based on the old behavior.
Adds a bunch of unit-tests for FLX sync where we test the new progress estimates for FLX sync under various circumstances.
Adds some integration tests for PBS/FLX where we make sure the progress notifications API works as expected when talking to baas and via AsyncOpen.
fixes a compile failure i didn't catch while constructing this PR initially. whoops.

☑️ 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-05-07T19:21:51Z

Pull Request Test Coverage Report for Build jonathan.reams_3219

Details

742 of 768 (96.61%) changed or added relevant lines in 4 files are covered.
78 unchanged lines in 18 files lost coverage.
Overall coverage decreased (-0.009%) to 90.806%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
test/object-store/sync/session/progress_notifications.cpp	692	718	96.38%

Files with Coverage Reduction	New Missed Lines	%
src/realm/query_engine.hpp	1	94.11%
src/realm/sort_descriptor.cpp	1	94.06%
src/realm/sync/noinst/server/server_history.cpp	1	63.7%
src/realm/util/file.cpp	1	78.73%
test/fuzz_tester.hpp	1	57.73%
test/test_index_string.cpp	1	93.48%
src/realm/mixed.cpp	2	86.46%
src/realm/sync/network/http.hpp	2	82.27%
test/object-store/sync/session/progress_notifications.cpp	2	97.13%
src/realm/object-store/sync/sync_session.cpp	3	91.67%

Totals
Change from base Build 2318:	-0.009%
Covered Lines:	214758
Relevant Lines:	236501

💛 - Coveralls

tgoyne

Looks reasonable at first glance, but I think I should take a proper pass over the tests and make sure that the expected behavior seems correct.

tgoyne · 2024-05-07T21:01:21Z

src/realm/object-store/sync/sync_session.cpp

@@ -1267,7 +1267,12 @@ void SyncSession::wait_for_download_completion(util::UniqueFunction<void(Status)
 uint64_t SyncSession::register_progress_notifier(std::function<ProgressNotifierCallback>&& notifier,
                                                 ProgressDirection direction, bool is_streaming)
 {
-    return m_progress_notifier.register_callback(std::move(notifier), direction, is_streaming);
+    int64_t pending_query_version = 0;
+    assert_mutex_unlocked();


register_progress_notifier() needs to be annotated as REQUIRES(!m_state_mutex) rather than this.

tgoyne · 2024-05-07T21:04:51Z

src/realm/object-store/sync/sync_session.cpp


-    started_notifying = true;
+        // If this is a non-streaming download progress update and this notifier was
+        // created for a later query version (i.e. we're currently downloading


e.g., not i.e.

tgoyne · 2024-05-07T21:07:12Z

src/realm/object-store/sync/sync_session.cpp

+        // The initial download size we get from the server is the uncompacted
+        // size, and so the download may complete before we actually receive
+        // that much data. When that happens, transferrable will drop and we


I think the mention of compaction here is out of date. transferable can still sometimes drop with BaaS, but it no longer due to compaction.

yeah, i dunno. it can also drop due to compaction with pbs. tbh, i don't know the full set of circumstances when this can happen, but i wanted to preserve the behavior.

isn't this actually also used for upload as a way to save the uploadable bytes at the time of registration (or when update is called first)? The change below also suggests this.

well, yeah, but why we do that exactly is not clear anymore.

tgoyne · 2024-05-07T21:10:19Z

test/object-store/sync/session/progress_notifications.cpp

+            CHECK(!callback_was_called);
+        }
+
+// The functionality of this test was moved out of the object-store and into the sync client.


I'm not sure what this comment means?

it means that we've removed the fake "server_version" parameter to the object-store progress callback and now the sync::Session itself ignores progress updates before it receives its first download message

realm-core/src/realm/sync/client.cpp

Line 1871 in c967ba1

return;

. we can probably remove this test and/or replace it with an integration test?

i've verified that we updated the sync::Session upload/download progress tests to so we don't get any progress updates until we've received at least one download message, and removed this test in a later commit.

Not getting any upload progress updates until we've received at least one download message is itself a breaking change that should be reverted. I think all of the changes to test_sync.cpp in #7124 that aren't just adapting to the changed signature of the progress notification callback are bogus and are resulting in some tests (such as Sync_UploadDownloadProgress_3) not testing the scenario they were specifically written to test.

Hm, maybe that's actually wrong and we never were sending upload notifications before the first download. The sync progress notification tests are definitely nonsense now, but it's sort of hard to tell if there's also functional changes.

Yeah, this is something I went back and forth in the PR about during the initial implementation. I think there is a functional change in the sync::Session behavior, but it is just behavior that used to be in object-store. You used to get an initial upload notification when you first registered your callback, but we ignored progress updates from the sync::Session until after you'd received the first download.

I can take a look at Sync_UploadDownloadProgress_3 and any other tests to try to make sure they're doing what they say they do, but probably not before this PR is merged.

danieltabacaru

Reviewed the functional changes. Tests are next (and last stop).

danieltabacaru · 2024-05-10T16:57:42Z

src/realm/object-store/sync/sync_session.cpp


-    started_notifying = true;
+        // If this is a non-streaming download progress update and this notifier was


I guess this check prevents reporting any progress until the latest query at the time the user registered the notifier starts bootstrapping right? I guess this is nice because there is no 0->1->0->....->1 anymore, but it may take a long time until the user gets any progress.

Yeah, for the most common case this means we skip query version zero and you get progress notifications for version 1. But yeah, in the worst case if you had a whole bunch of outstanding subscription sets you could end up having to wait for them all to sync.

src/realm/object-store/sync/sync_session.cpp

danieltabacaru · 2024-05-10T18:10:56Z

src/realm/object-store/sync/sync_session.cpp


+    // A notifier is expired if at least as many bytes have been transferred
+    // as were originally considered transferrable.
+    is_expired =


If I understand it correctly, this also fires when there is a fresh realm with no subscriptions and the schema is downloaded (as opposed to previous implementation) right? If that's the case it could be worth mentioning it in the description. Edit: the check at line 1605 actually prevents this.

danieltabacaru · 2024-05-10T21:14:04Z

test/object-store/sync/session/progress_notifications.cpp

-            callback_was_called = false;
-            progress.update_upload(2, 2, 1);
-            CHECK_FALSE(callback_was_called);
+            progress.register_callback(


should we add a check after registration?

danieltabacaru · 2024-05-10T21:22:21Z

test/object-store/sync/session/progress_notifications.cpp

-        bool is_streaming = GENERATE(false, true);
-        progress.update(0, 0, 0, 0, 1);
+    SECTION("callback is invoked after each update for streaming notifiers") {
+        progress.update(0, 0, 0, 0, 1, 0, 0, 0);


nit: it helps readability when using double values (eg, 0.0)

danieltabacaru · 2024-05-10T21:27:34Z

test/object-store/sync/session/progress_notifications.cpp

-            if (!is_streaming)
-                register_default_upload_callback(is_streaming);
-
+            double current_estimate = current_transferred / double(current_transferrable);


nit: you could use an std::vectorstd::pair> and iterate through it updating the progress and do the checks. same for a few more tests below.

yeah. i think there's real improvements to be done in the readability/maintainability of these tests, but for now I want to get them back to a known good state before doing refactoring.

danieltabacaru · 2024-05-10T21:32:11Z

test/object-store/sync/session/progress_notifications.cpp

+                    callback_was_called = true;
+                },
+                NotifierType::upload, false, 0);
+            // Wait for the initial callback.


this doesn't really wait for anything

I've removed the comment - it was just copied over from the old tests.

danieltabacaru · 2024-05-10T21:36:05Z

test/object-store/sync/session/progress_notifications.cpp


            // Second callback
            callback_was_called = false;
            current_transferred = 79;
            current_transferrable = 1021;
-            progress.update_upload(current_transferred, current_transferrable, 1);
+            current_estimate = current_transferred / double(current_transferrable);
+            progress.update(68, 191, current_transferred, current_transferrable, 1, 68 / double(191),


nit: you could increase the snapshot version in these subsequent calls. something like local_version++ would do.

these tests were copied from before the latest changes, and i'd prefer to keep them as close as possible to that in this PR and improve them when we're satisfied there aren't further regressions.

danieltabacaru · 2024-05-10T21:40:40Z

test/object-store/sync/session/progress_notifications.cpp

-            progress.update(current_downloaded, current_downloadable, current_uploaded, current_uploadable, 1);
+            current_estimate = current_downloaded / double(current_downloadable);
+            progress.update(current_downloaded, current_downloadable, current_uploaded, current_uploadable, 1,
+                            current_estimate, 1.0, 0);


how can the upload estimate be 1.0? i think it should be 31/329

danieltabacaru · 2024-05-10T22:07:04Z

src/realm/object-store/sync/sync_session.cpp

+        // the total number of uploadable bytes available rather than the number of
+        // bytes this NotifierPackage was waiting to upload.
+        if (!is_download) {
+            estimate = std::min(transfered / double(transferable), 1.0);


nice catch! but you need to check transferable is greater than 0. Running for upload notifications, with no data transfer ongoing test and checking the estimate reveals it as NaN.

danieltabacaru · 2024-05-10T22:13:31Z

src/realm/object-store/sync/sync_session.cpp

+        // The initial download size we get from the server is the uncompacted
+        // size, and so the download may complete before we actually receive
+        // that much data. When that happens, transferrable will drop and we


isn't this actually also used for upload as a way to save the uploadable bytes at the time of registration (or when update is called first)? The change below also suggests this.

danieltabacaru · 2024-05-10T22:43:12Z

test/object-store/sync/session/progress_notifications.cpp

-            register_default_upload_callback();
+            progress.register_callback(
+                [&](auto, auto, double) {
+                    callback_was_called = true;


as mentioned above, the estimate here is NaN

danieltabacaru · 2024-05-10T22:50:28Z

test/object-store/sync/session/progress_notifications.cpp

+            CHECK(callback_was_called);
+            CHECK(transferred == current_transferred);
+            CHECK(transferrable == original_transferrable);
+            CHECK(upload_estimate == std::min(1.0, current_transferred / double(original_transferrable)));


why use the minimum? since current_transferred > original_transferrable, the estimate is 1.0. If anything, i'd add a comment

i've updated this to just CHECK(upload_estimate == 1.0)since the sync session notification won't let you have an estimate greater than 1.0 for non-streaming upload notifications

…sion

jbreams added 3 commits May 7, 2024 10:52

Make PBS progress notifications work the way they used to

31aa243

Add unit tests for flx sync progress notifications

3024fc7

Add integration tests

99605de

cla-bot bot added the cla: yes label May 7, 2024

github-actions bot assigned jbreams May 7, 2024

jbreams changed the title ~~Restore progress notifier behavior when sync session is already caught up~~ RCORE-2099 Restore progress notifier behavior when sync session is already caught up May 7, 2024

fix swift build

45aa3af

jbreams requested review from tgoyne and danieltabacaru May 7, 2024 19:24

jbreams marked this pull request as ready for review May 7, 2024 19:24

tgoyne reviewed May 7, 2024

View reviewed changes

jbreams added 4 commits May 7, 2024 18:33

fixes for thomas

ab82d79

make non-streaming notifier actually non-streaming

8e0248c

make noop non-streaming upload notifier check harder

2681a3d

remove defunct test

ebc3728

tgoyne approved these changes May 8, 2024

View reviewed changes

danieltabacaru reviewed May 10, 2024

View reviewed changes

jbreams added 2 commits May 20, 2024 11:24

Fixes for daniel

458da4b

add a few more checks

187c6c8

jbreams requested a review from danieltabacaru May 20, 2024 15:38

add missing assertion

c199c6f

danieltabacaru approved these changes May 20, 2024

View reviewed changes

jbreams added 3 commits May 20, 2024 13:05

Merge remote-tracking branch 'origin/master' into jbr/progress_regres…

396a820

…sion

changelog

b97054b

fix windows warning

7b4b429

jbreams merged commit c280bdb into master May 20, 2024
38 checks passed

jbreams deleted the jbr/progress_regression branch May 20, 2024 20:01

fealebenpae mentioned this pull request May 23, 2024

Prepare for 14.8.0 #7728

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jun 19, 2024


		started_notifying = true;
		// If this is a non-streaming download progress update and this notifier was

RCORE-2099 Restore progress notifier behavior when sync session is already caught up #7681

RCORE-2099 Restore progress notifier behavior when sync session is already caught up #7681

Conversation

jbreams commented May 7, 2024 • edited Loading

What, How & Why?

☑️ ToDos

coveralls-official bot commented May 7, 2024 • edited Loading

Pull Request Test Coverage Report for Build jonathan.reams_3219

Details

💛 - Coveralls

tgoyne 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieltabacaru 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

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

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

jbreams commented May 7, 2024 •

edited

Loading

coveralls-official bot commented May 7, 2024 •

edited

Loading