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.

Sign up for GitHub

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

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

Merged
merged 6 commits into from
Sep 15, 2021
+133 −12
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1ca0865
Workaround issue when initiating sync on a copied realm file
jedelbo Aug 25, 2021
a00e55a
Misc changes
jedelbo Aug 30, 2021
0e9713a
Make internal sync server accept pre-downloaded file
jedelbo Aug 30, 2021
c38b169
Merge branch 'master' into je/fix-write-copy
jedelbo Aug 31, 2021
adb9155
Update after review
jedelbo Sep 10, 2021
2c942d1
Another update after review
jedelbo Sep 13, 2021
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
4 changes: 2 additions & 2 deletions src/realm/exec/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

DEBUG_POSTFIX ${CMAKE_DEBUG_POSTFIX}
)
target_link_libraries(RealmBrowser Storage)

add_executable(Realm2JSON realm2json.cpp )
set_target_properties(Realm2JSON PROPERTIES
OUTPUT_NAME "realm2json-6"
OUTPUT_NAME "realm2json-10"
DEBUG_POSTFIX ${CMAKE_DEBUG_POSTFIX}
)
target_link_libraries(Realm2JSON Storage)
Expand Down
12 changes: 8 additions & 4 deletions src/realm/sync/noinst/client_history_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,8 +811,10 @@ void ClientHistoryImpl::update_sync_progress(const SyncProgress& progress,
version_type(root.get_as_ref_or_tagged(s_progress_download_client_version_iip).get_as_int()));
REALM_ASSERT(progress.upload.client_version >=
version_type(root.get_as_ref_or_tagged(s_progress_upload_client_version_iip).get_as_int()));
REALM_ASSERT(progress.upload.last_integrated_server_version >=
version_type(root.get_as_ref_or_tagged(s_progress_upload_server_version_iip).get_as_int()));
if (progress.upload.last_integrated_server_version > 0) {
REALM_ASSERT(progress.upload.last_integrated_server_version >=
version_type(root.get_as_ref_or_tagged(s_progress_upload_server_version_iip).get_as_int()));
}

auto uploaded_bytes = std::uint_fast64_t(root.get_as_ref_or_tagged(s_progress_uploaded_bytes_iip).get_as_int());
auto previous_upload_client_version =
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

root.set(s_progress_upload_server_version_iip,
RefOrTagged::make_tagged(progress.upload.last_integrated_server_version)); // Throws
}
if (downloadable_bytes) {
root.set(s_progress_downloadable_bytes_iip,
RefOrTagged::make_tagged(*downloadable_bytes)); // Throws
Expand Down
16 changes: 13 additions & 3 deletions src/realm/sync/noinst/server_history.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -850,8 +850,16 @@ auto ServerHistory::do_bootstrap_client_session(SaltedFileIdent client_file_iden
if (download_progress.server_version > current_server_version)
return BootstrapError::bad_download_server_version;
auto last_integrated_client_version = version_type(m_acc->cf_client_versions.get(client_file_index));
if (download_progress.last_integrated_client_version > last_integrated_client_version)
return BootstrapError::bad_download_client_version;
if (download_progress.last_integrated_client_version > last_integrated_client_version) {
if (last_integrated_client_version == 0) {
// We assume we are booting on a pre-downloaded client file
last_integrated_client_version = download_progress.last_integrated_client_version;
recip_hist_base_version = download_progress.server_version;
}
else {
return BootstrapError::bad_download_client_version;
}
}

// Validate `server_version`
{
Expand Down Expand Up @@ -1014,7 +1022,9 @@ bool ServerHistory::fetch_download_info(file_ident_type client_file_ident, Downl
download_progress = download_progress_2;
cumulative_byte_size_current = std::uint_fast64_t(cumulative_byte_size_current_2);
cumulative_byte_size_total = std::uint_fast64_t(cumulative_byte_size_total_2);
upload_progress = UploadCursor{upload_client_version, upload_server_version};
if (upload_client_version > upload_progress.client_version) {
upload_progress = UploadCursor{upload_client_version, upload_server_version};
}

return true;
}
Expand Down
2 changes: 1 addition & 1 deletion src/realm/sync/server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3637,7 +3637,7 @@ class Session final : private FileIdentReceiver {
bool body_is_compressed = false;
version_type end_version = last_server_version.version;
DownloadCursor download_progress;
UploadCursor upload_progress = {0, 0};
UploadCursor upload_progress = m_upload_threshold;
std::uint_fast64_t downloadable_bytes = 0;
std::size_t num_changesets;
std::size_t accum_original_size;
Expand Down
10 changes: 8 additions & 2 deletions test/object-store/realm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,13 @@ TEST_CASE("Get Realm using Async Open", "[asyncOpen]") {
std::lock_guard<std::mutex> lock(mutex);
return bool(realm_ref);
});
// Write some data
SharedRealm realm = Realm::get_shared_realm(std::move(realm_ref));
realm->begin_transaction();
realm->read_group().get_table("class_object")->create_object_with_primary_key(2);
realm->commit_transaction();
wait_for_upload(*realm);
wait_for_download(*realm);
client_file_id = realm->read_group().get_sync_file_id();
realm->write_copy(config3.path, BinaryData());
}
Expand All @@ -582,7 +588,7 @@ TEST_CASE("Get Realm using Async Open", "[asyncOpen]") {
wait_for_download(*realm);
// Make sure we have got a new client file id
REQUIRE(realm->read_group().get_sync_file_id() != client_file_id);
REQUIRE(realm->read_group().get_table("class_object")->size() == 2);
REQUIRE(realm->read_group().get_table("class_object")->size() == 3);

// Check that we can continue committing to this realm
realm->begin_transaction();
Expand All @@ -593,7 +599,7 @@ TEST_CASE("Get Realm using Async Open", "[asyncOpen]") {
// Check that this change is now in the original realm
wait_for_download(*origin);
origin->refresh();
REQUIRE(origin->read_group().get_table("class_object")->size() == 3);
REQUIRE(origin->read_group().get_table("class_object")->size() == 4);
}

SECTION("downloads Realms which exist on the server") {
Expand Down
121 changes: 121 additions & 0 deletions test/object-store/sync/app.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include <realm/object-store/sync/mongo_database.hpp>
#include <realm/object-store/sync/mongo_collection.hpp>
#include <realm/object-store/sync/sync_session.hpp>
#include <realm/object-store/thread_safe_reference.hpp>

#include "util/baas_admin_api.hpp"
#include "util/event_loop.hpp"
Expand Down Expand Up @@ -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)() = [] {
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 replace lines 1781-1798 with a call to get_integration_config()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

return std::unique_ptr<GenericNetworkTransport>(new IntTestTransport);
};
std::string base_url = get_base_url();
REQUIRE(!base_url.empty());
auto app_session = get_runtime_app_session(base_url);

auto app_id = app_session.client_app_id;
auto config = App::Config{app_id,
factory,
base_url,
util::none,
Optional<std::string>("A Local App Version"),
util::none,
"Object Store Platform Tests",
"Object Store Platform Version Blah",
"An sdk version"};

auto base_path = util::make_temp_dir();
util::try_remove_dir_recursive(base_path);
util::try_make_dir(base_path);
util::try_make_dir(base_path + "/orig");
util::try_make_dir(base_path + "/copy");

// Create realm file without client file id
{
TestSyncManager sync_manager(TestSyncManager::Config(config), {});
auto app = sync_manager.app();
app->log_in_with_credentials(AppCredentials::anonymous(),
[&](std::shared_ptr<SyncUser> user, Optional<app::AppError> error) {
REQUIRE(!error);
REQUIRE(user);
});

ThreadSafeReference realm_ref;

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the default value works fine here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

realm_config.sync_config->error_handler = [](std::shared_ptr<SyncSession>, SyncError error) {
std::cerr << error.message << std::endl;
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 override the sync handler here? Should we assert that there is no error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

};
realm_config.schema_version = 1;
realm_config.path = base_path + "/orig/default.realm";

std::mutex mutex;
auto task = realm::Realm::get_synchronized_realm(realm_config);
task->start([&](ThreadSafeReference ref, std::exception_ptr error) {
std::lock_guard<std::mutex> lock(mutex);
REQUIRE(!error);
realm_ref = std::move(ref);
});
util::EventLoop::main().run_until([&] {
std::lock_guard<std::mutex> lock(mutex);
return bool(realm_ref);
});
SharedRealm realm = Realm::get_shared_realm(std::move(realm_ref));

// Write some data
realm->begin_transaction();
CppContext c;
Object::create(c, realm, "Person",
util::Any(realm::AnyDict{{"_id", util::Any(ObjectId::gen())},
{"age", INT64_C(64)},
{"firstName", std::string("Paul")},
{"lastName", std::string("McCartney")}}));
realm->commit_transaction();
wait_for_upload(*realm);
wait_for_download(*realm);

realm->write_copy(base_path + "/copy/default.realm", BinaryData());

// Write some additional data
realm->begin_transaction();
Object::create(c, realm, "Dog",
util::Any(realm::AnyDict{{"_id", util::Any(ObjectId::gen())},
{"breed", std::string("stabyhoun")},
{"name", std::string("albert")},
{"realm_id", std::string("foo")}}));
realm->commit_transaction();
wait_for_upload(*realm);
}
// Starting a new session based on the copy
{
TestSyncManager sync_manager(TestSyncManager::Config(config), {});
auto app = sync_manager.app();
app->log_in_with_credentials(AppCredentials::anonymous(),
[&](std::shared_ptr<SyncUser> user, Optional<app::AppError> error) {
REQUIRE(!error);
REQUIRE(user);
});

ThreadSafeReference realm_ref;

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;
realm_config.sync_config->error_handler = [](std::shared_ptr<SyncSession>, SyncError error) {
std::cerr << error.message << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

};
realm_config.schema_version = 1;
realm_config.path = base_path + "/copy/default.realm";

SharedRealm realm = realm::Realm::get_shared_realm(realm_config);
wait_for_download(*realm);

// Check that we can continue committing to this realm
realm->begin_transaction();
CppContext c;
Object::create(c, realm, "Dog",
util::Any(realm::AnyDict{{"_id", util::Any(ObjectId::gen())},
{"breed", std::string("bulldog")},
{"name", std::string("fido")},
{"realm_id", std::string("foo")}}));
realm->commit_transaction();
wait_for_upload(*realm);
}
}

TEST_CASE("app: sync integration", "[sync][app]") {
const auto schema = Schema{{"Dog",
{{"_id", PropertyType::ObjectId | PropertyType::Nullable, true},
Expand Down
5 changes: 5 additions & 0 deletions tools/run_baas.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#!/usr/bin/env bash

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}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will rename the script.