Workaround issue when initiating sync on a copied realm file #4878

jedelbo · 2021-08-27T11:37:54Z

The problem is that the server does not know that the client has already
integrated some server versions, and therefore it sends down 0 as
progress/upload/last_integrated_server_version. On the client this number
may be bigger and in this case that number will not be overwritten with 0.

On the server we have to change the handling of the upload cursor in case
a newly created client indicates that is has already integrated some server
changesets.

What, How & Why?

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)

The problem is that the server does not know that the client has already integrated some server versions, and therefore it sends down 0 as progress/upload/last_integrated_server_version. On the client this number may be bigger and in this case that number will not be overwritten with 0. There may still be issues on the server, however.

If the IDENT message indicates that the newly created client has already integraded some server changesets, adjust the upload threshold accordingly- If the threshold is ahead of the upload cursor stored in the history, use the threshold values.

jbreams · 2021-08-31T13:53:59Z

src/realm/sync/noinst/client_history_impl.cpp

@@ -829,8 +831,10 @@ void ClientHistoryImpl::update_sync_progress(const SyncProgress& progress,
             RefOrTagged::make_tagged(progress.latest_server_version.salt)); // Throws
    root.set(s_progress_upload_client_version_iip,
             RefOrTagged::make_tagged(progress.upload.client_version)); // Throws
-    root.set(s_progress_upload_server_version_iip,
-             RefOrTagged::make_tagged(progress.upload.last_integrated_server_version)); // Throws
+    if (progress.upload.last_integrated_server_version > 0) {


If the current version of s_progres_upload_server_version_iip is zero, would this be a noop without this conditional?

I am not sure I understand the question. If current value is zero and progress value is zero then it is obviously a noop. The test just makes sure that we will not overwrite a non-zero value with a zero.

I think you are answering my question without understanding it 😄 . We were changing this from unconditionally setting a value to conditionally setting a value, and I wanted to verify the preconditions. Can you add a comment about why we're conditionally setting this here?

jbreams · 2021-08-31T13:55:49Z

test/object-store/sync/app.cpp

@@ -1776,6 +1777,126 @@ TEST_CASE("app: set new embedded object", "[sync][app]") {
    }
 }

+TEST_CASE("app: make distributable client file", "[sync][app]") {
+    std::unique_ptr<GenericNetworkTransport> (*factory)() = [] {


You can replace lines 1781-1798 with a call to get_integration_config()

jbreams · 2021-08-31T13:56:58Z

test/object-store/sync/app.cpp

+        realm_config.sync_config = std::make_shared<realm::SyncConfig>(app->current_user(), bson::Bson("foo"));
+        realm_config.sync_config->client_resync_mode = ClientResyncMode::Manual;
+        realm_config.sync_config->error_handler = [](std::shared_ptr<SyncSession>, SyncError error) {
+            std::cerr << error.message << std::endl;


Do we need to override the sync handler here? Should we assert that there is no error?

jbreams · 2021-08-31T13:58:30Z

test/object-store/sync/app.cpp

+
+        realm::Realm::Config realm_config;
+        realm_config.sync_config = std::make_shared<realm::SyncConfig>(app->current_user(), bson::Bson("foo"));
+        realm_config.sync_config->client_resync_mode = ClientResyncMode::Manual;


I think the default value works fine here

jbreams · 2021-08-31T17:26:00Z

test/object-store/sync/app.cpp

+        realm_config.sync_config = std::make_shared<realm::SyncConfig>(app->current_user(), bson::Bson("foo"));
+        realm_config.sync_config->client_resync_mode = ClientResyncMode::Manual;
+        realm_config.sync_config->error_handler = [](std::shared_ptr<SyncSession>, SyncError error) {
+            std::cerr << error.message << std::endl;


I think we want to abort if we get here because that's how we'll know if the server sent us an error message, or am I missing something?

jbreams · 2021-08-31T17:27:54Z

tools/run_baas.sh

+
+PROJECT_DIR=$(git rev-parse --show-toplevel)
+MDBREALM_TEST_SERVER_TAG=$(grep MDBREALM_TEST_SERVER_TAG ${PROJECT_DIR}/dependencies.list |cut -f 2 -d=)
+docker run --rm -p 9090:9090 -v ~/.aws/credentials:/root/.aws/credentials -it docker.pkg.github.com/realm/ci/mongodb-realm-test-server:${MDBREALM_TEST_SERVER_TAG}


If you want, you can also just run ./evergreen/install_baas.sh -w baas-work-dir with your AWS credentials in the correct environment variables and it will the latest version of baas deployed to prod.

ok, but I just wanted a script that starts the version we refer to in the dependencies. I think that is the one that is run in Jenkins CI.

TBH, I think we should get rid of the dependencies.list for deciding what version of baas to run. It's chronically out of date, sometimes by months or quarters. Can you change the name of this file to be something like run_baas_docker_image.sh to clarify what it's actually doing? Otherwise I don't have an objection to small utility scripts like this, just wanted you to know that there's already something in the codebase that will accomplish the same task.

If I run ./evergreen/install_baas.sh -w baas-work-dir, I get Unsupported platform linuxmint 19.3

I will rename the script.

jbreams · 2021-08-31T17:28:39Z

src/realm/exec/CMakeLists.txt

@@ -24,14 +24,14 @@ target_link_libraries(RealmTrawler Storage)

 add_executable(RealmBrowser EXCLUDE_FROM_ALL realm_browser.cpp )
 set_target_properties(RealmBrowser PROPERTIES
-    OUTPUT_NAME "realm-browser-6"
+    OUTPUT_NAME "realm-browser-10"


Can we just remove these suffixes? What value are they adding?

They are adding the value that It is build into the name which version it can handle. I have to have multiple versions around so that I can open whatever realm file I get from a customer.

Ah, if it's part of your workflow then okay. I wish this were configurable. We use these tools outside of your debugging processes - e.g. the cloud team depends on them. @tkaye407 , will you need to update anything if we rename realm2json like this?

We currently just pin to a specific version (though we probably should be able to pick up the latest merge), but once this goes in I can update the version / name of the executable that we link to in our evergreen config. Thanks for pointing out

The problem is that the server does not know that the client has already integrated some server versions, and therefore it sends down 0 as progress/upload/last_integrated_server_version. On the client this number may be bigger and in this case that number will not be overwritten with 0. On the server side, the following behavior is expected: If the IDENT message indicates that the newly created client has already integraded some server changesets, adjust the upload threshold accordingly- If the threshold is ahead of the upload cursor stored in the history, use the threshold values.

jedelbo force-pushed the je/fix-write-copy branch from d5fc377 to 32963a5 Compare August 27, 2021 11:39

jedelbo force-pushed the je/fix-write-copy branch from 32963a5 to 1ca0865 Compare August 27, 2021 11:58

cmelchior mentioned this pull request Aug 27, 2021

Add support for SyncConfiguration.assetFile realm/realm-java#7556

Merged

jedelbo added 2 commits August 30, 2021 09:10

Misc changes

a00e55a

jedelbo requested review from jbreams and simonask August 30, 2021 09:55

jedelbo marked this pull request as ready for review August 30, 2021 09:56

jedelbo mentioned this pull request Aug 30, 2021

WriteCopyTo cause "Bad sync progress received" for Synced Realms #4872

Closed

Merge branch 'master' into je/fix-write-copy

c38b169

jbreams requested changes Aug 31, 2021

View reviewed changes

kneth mentioned this pull request Sep 1, 2021

MongoDB Realm Cloud sync fails with "Bad sync process received (6)" error realm/realm-js#3508

Closed

DominicFrei mentioned this pull request Sep 7, 2021

Add two tests to verify bundled Sync Realms can actually sync. realm/realm-dotnet#2611

Merged

2 tasks

jedelbo requested a review from jbreams September 10, 2021 12:23

Update after review

adb9155

jedelbo force-pushed the je/fix-write-copy branch from 85b0c5b to adb9155 Compare September 10, 2021 14:57

Another update after review

2c942d1

jbreams approved these changes Sep 14, 2021

View reviewed changes

jedelbo merged commit fb12944 into master Sep 15, 2021

jedelbo deleted the je/fix-write-copy branch September 15, 2021 09:46

github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workaround issue when initiating sync on a copied realm file #4878

Workaround issue when initiating sync on a copied realm file #4878

jedelbo commented Aug 27, 2021 •

edited

Loading

jbreams Aug 31, 2021

jedelbo Sep 10, 2021

jbreams Sep 12, 2021

jbreams Aug 31, 2021

jedelbo Sep 10, 2021

jbreams Aug 31, 2021

jedelbo Sep 10, 2021

jbreams Aug 31, 2021

jedelbo Sep 10, 2021

jbreams Aug 31, 2021

jedelbo Sep 10, 2021

jbreams Aug 31, 2021

jedelbo Sep 10, 2021

jbreams Sep 12, 2021

jedelbo Sep 13, 2021

jedelbo Sep 13, 2021

jbreams Aug 31, 2021

jedelbo Sep 10, 2021

jbreams Sep 12, 2021

tkaye407 Sep 13, 2021

Workaround issue when initiating sync on a copied realm file #4878

Workaround issue when initiating sync on a copied realm file #4878

Conversation

jedelbo commented Aug 27, 2021 • edited Loading

What, How & Why?

☑️ ToDos

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

jedelbo commented Aug 27, 2021 •

edited

Loading