Add migrated state information to flexible sync client BIND message #6464

michael-wb · 2023-03-31T19:47:44Z

What, How & Why?

Updated the BIND message for flexible sync to repurpose the server_path field to use as stringified JSON data. If the client is in the FLX migrated state, the original PBS partition value will be added to the JSON data section of the FLX BIND message.
Added unit tests for the message generation portion of the ProtocolCodec class.

Fixes #6442

☑️ ToDos

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

jbreams · 2023-03-31T21:34:05Z

src/realm/sync/noinst/client_impl_base.cpp

+    std::string path_data;
+    if (m_is_flx_sync_session) {
+        nlohmann::json bind_json_data;
+        if (protocol_version >= 8) {


I think this kind of protocol version specific logic should really go in the protocol codec. It's the thing that decides what version/mode of the wire protocol results in which bytes get put on the wire.

I actually had it as two different functions with the FLX one taking a json object, but changed it to this since all the other functions just took strings. I'll change it back.

jbreams · 2023-03-31T21:38:49Z

src/realm/sync/noinst/migration_store.cpp

+    if (m_state != MigrationState::Migrated) {
+        return std::nullopt;
+    }
+    return m_migrated_partition;


I'm just realizing that this function and get_query_string() are returning a string_view which is an unowned view of a std::string protected by a lock but don't guarantee that the lock is held after returned, so it's possible to call get_migrated_partition() and have the returned string_view pointing to free'd or otherwise invalid memory after return.

I think both this and get_query_string() need to return a std::optionalstd::string

jbreams · 2023-03-31T21:40:28Z

src/realm/sync/noinst/protocol_codec.hpp

-    void make_bind_message(int protocol_version, OutputBuffer&, session_ident_type session_ident,
-                           const std::string& server_path, const std::string& signed_user_token,
-                           bool need_client_file_ident, bool is_subserver);
+    void make_bind_message(OutputBuffer&, session_ident_type session_ident, const std::string& path_data,


I think we may need a FLX/PBS specific version of these functions like we have with IDENT messages to handle the different formats/protocol version handling here.

Added the two different versions back and updated the tests.

jbreams · 2023-03-31T21:45:00Z

test/test_sync_protocol_codec.cpp

+template <class A, class B>
+bool compare_out_string(const A& expected, const B& out, const std::shared_ptr<util::Logger>& logger)
+{
+    if (expected.size() == out.size() && memcmp(expected.data(), out.data(), out.size()) == 0) {


if you wanted to make this a little more idiomatic, at least to my eyes you could do something like

template <class A, class B> void check_out_string(const A& expected, const B& out, unit_test::TestContext& test_context) { CHECK_EQUAL(std::string_view(std::data(expected), std::size(expected)), std::string_view(std::data(out), std::size(out))); }

Then the CHECK_EQUAL macro would just work as expected.

That works great - thanks for the recommendation

jbreams · 2023-03-31T21:53:01Z

src/realm/sync/client.hpp

@@ -333,6 +333,10 @@ class Session {
        /// changeset data in a single integration attempt.
        size_t flx_bootstrap_batch_size_bytes = 1024 * 1024;

+        /// Contains the original PBS partition value from before the migration -
+        /// empty if not migrated
+        std::optional<std::string_view> migrated_partition;


Should this really just be a std::shared_ptr and then the sync client can just query this as needed?

Only the original string value (as an optional) is necessary and I didn't want to extend the lifetime of the migration_store beyond SyncSession. I can change it if you think it would be better.

I just updated it to be a reference to the migration store, since Daniels PR needs the store for creating the subscriptions and completing the migration. I used the same changes from his review.

jbreams · 2023-03-31T21:53:29Z

test/test_sync_protocol_codec.cpp

@@ -0,0 +1,224 @@
+/*************************************************************************
+ *
+ * Copyright 2016 Realm Inc.


Oops - definitely a copy/paste artifact from reusing another test file as a starting point....

danieltabacaru · 2023-04-04T15:54:57Z

The github issue this PR fixes mentions userRequestedPBS to be added as part of BIND. Is that work done in a different pr? I need it for #6460.

michael-wb · 2023-04-04T17:52:09Z

The github issue this PR fixes mentions userRequestedPBS to be added as part of BIND. Is that work done in a different pr? I need it for #6460.

It was removed since the presence of the partition value is enough to imply "userRequestedPBS". The migrated partition value is an optional string that will only be populated when the client is in the migrated FLX state. I will update the issue.

src/realm/sync/noinst/client_impl_base.cpp

src/realm/sync/noinst/migration_store.cpp

…g-updates Adjusted saving of migrated partition based on new changes

…g-updates

jbreams

Now that the server has support for backfilling partition values, can we add a test to verify that sending this info in the bind message actually does what we expect on the server side?

jbreams · 2023-04-06T20:24:43Z

src/realm/sync/noinst/client_impl_base.hpp

@@ -750,7 +751,7 @@ class ClientImpl::Session {

    /// Update internal client state when a flx subscription becomes complete outside
    /// of the normal sync process. This can happen during client reset.
-    void non_sync_flx_completion(int64_t version);
+    void on_sync_flx_completion(int64_t version);


wow, what a typo!

Judging by the comment it should be prefixed with non

so on_non_sync_flx_completion? or just revert it...

I think on_non_sync_flx_completion does sound better

Since it's calling on_flx_sync_version_complete() on the wrapper, I updated the name to be the same.

jbreams · 2023-04-06T20:38:13Z

src/realm/sync/noinst/protocol_codec.cpp

+        json_data_stg = json_data.dump();
+    }
+
+    static_cast<void>(protocol_version);


no need to static_cast this here. we do it in the pbs function just to avoid an unused variable warning, but we do use it here. we can consider just removing it as a parameter to the pbs version to simplify things a bit.

…g-updates

jbreams

Neat, just a few questions.

jbreams · 2023-04-11T17:18:12Z

src/realm/sync/noinst/migration_store.cpp

@@ -11,7 +11,7 @@ constexpr static std::string_view c_flx_migration_started_at("flx_migration_star
 constexpr static std::string_view c_flx_migration_completed_at("flx_migration_completed_at");
 constexpr static std::string_view c_flx_migration_state("flx_migration_state");
 constexpr static std::string_view c_flx_migration_query_string("flx_migration_query_string");
-
+constexpr static std::string_view c_flx_migration_original_partition("flx_migration_original_partition");


i'm just noticing this now, and we def don't need to change it if you and @danieltabacaru like it this way, but these names will end up with a table called flx_migration and then columns named flx_migration_started_at. so in json it'll look like {flx_migration_started_at: timestamp, flx_migration_state: state, flx_migration_query_string: "foo=bar", flx_migration_original_partition: "bar", flx_migration_completed_at: timestamp} - did we mean for it to look more like {started_at: timestamp, state: state, query_string: "foo=bar", original_partition="bar", completed_at: timestamp}?

Sure - I can update that now

jbreams · 2023-04-11T17:20:42Z

CHANGELOG.md

@@ -20,6 +20,7 @@
 * Fix compiler warnings with MacOS Clang 14.0.3 ([PR #6467](https://github.com/realm/realm-core/pull/6467))
 * Perform a client reset to migrate a sync'd realm from PBS to FLX and vice versa ([#6393](https://github.com/realm/realm-core/issues/6393))
 * The following unused util headers have been deleted: call_with_tuple.hpp, get_file_size.hpp, inspect.hpp, substitute.hpp, type_list.hpp, and utf8.hpp.
+* Add migrated state information to flexible sync client BIND message ([PR #6464](https://github.com/realm/realm-core/pull/6464))


Can we add something about how this will support the server back-filling partition values that aren't in your schema for migrated PBS realms? Just something so SDKs/customers know how this change actually effects them?

src/realm/sync/noinst/migration_store.cpp

…ing FLX

danieltabacaru · 2023-04-12T13:10:29Z

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

                save_sync_config_after_migration();
                download_fresh_realm(error.server_requests_action);
                return;
            case sync::ProtocolErrorInfo::Action::RevertToPBS:
                // If the client was updated to use FLX natively, but the server was rolled back to PBS,
-                // propagate the error up to the user
+                // the server should be sending "SwitchToFLX", throw exception if this error is received.


SwitchToPBS

danieltabacaru · 2023-04-12T13:14:05Z

src/realm/sync/noinst/client_impl_base.cpp

+        std::string server_path = get_virt_path();
+        logger.debug("Sending: BIND(session_ident=%1, need_client_file_ident=%2 is_subserver=%3 server_path=%4)",
+                     session_ident, need_client_file_ident, is_subserver, server_path);
+        protocol.make_pbs_bind_message(protocol_version, out, session_ident, server_path, empty_access_token,


unrelated: is there a plan, at some point, to make pbs work with a json so we don't need to treat it differently than flx?

I don't believe so - the JSON data was "shimmed" into the BIND message for FLX, since the existing server_path section, which is used to send the partition value in PBS, was not used by FLX. In addition, since we want to move away from PBS eventually, there likely won't be a need for adding JSON values.

danieltabacaru · 2023-04-12T13:17:30Z

src/realm/sync/noinst/migration_store.cpp

            REALM_ASSERT(m_state == state);
            REALM_ASSERT(m_query_string == query_string);
+            REALM_ASSERT(m_migrated_partition == migrated_partition);


danieltabacaru · 2023-04-12T13:24:02Z

src/realm/sync/protocol.hpp

@@ -34,6 +34,10 @@ namespace sync {
 //   7 Client takes the 'action' specified in the 'json_error' messages received
 //     from server. Client sends 'json_error' messages to the server.
 //
+//   8 Support for PBS->FLX client migration and websocket http errors as websocket


nit: we could split the first sentence in two since it's different things

danieltabacaru · 2023-04-12T13:36:42Z

src/realm/sync/noinst/client_impl_base.cpp

+    if (m_is_flx_sync_session) {
+        nlohmann::json bind_json_data;
+        if (auto migrated_partition = get_migration_store()->get_migrated_partition()) {
+            bind_json_data["migratedPartition"] = *migrated_partition;


so server_path is not needed anymore for flx bind?

It was not being used - the call to get_virt_path() returns the partition value provided in SyncConfig, which is always an empty string for FLX sync.

danieltabacaru · 2023-04-12T13:38:53Z

src/realm/sync/noinst/protocol_codec.cpp

+{
+    std::string json_data_stg;
+    // Protocol version v8 and above accepts stringified json_data for the first data argument
+    if (protocol_version >= 8 && !json_data.empty()) {


is the first check actually needed? I guess this should only be released once the v8 is enabled.

Side note: Is this suppose to work with a server which does not support v8? As it stands, we are not going to send the server_path so I don't know if that's a problem or not (for what is worth, we don't send the server_path either for >= v8)

I don't think so, since BAAS is the only server used with FLX sync, it will always be up to date to the latest protocol. It may start to get tricky once we have to work with tiered sync, since we can't rely on the server being up to the latest version.
We can remove this, but I'd prefer to do it once we commit to start using protocol v8 once everything is integrated.

fair enough. let's make sure we don't forget to remove it then

I added a comment to #6342 to remove this.

Added JSON data field to BIND message for migratedPartition value

c3e7128

cla-bot bot added the cla: yes label Mar 31, 2023

github-actions bot assigned michael-wb Mar 31, 2023

Updated changelog

fd328a3

michael-wb requested review from jbreams and danieltabacaru March 31, 2023 19:49

Use correct (fill) constructor for string

2a6bdb5

jbreams reviewed Mar 31, 2023

View reviewed changes

Updates from review

a14058f

michael-wb requested a review from kmorkos April 4, 2023 05:41

Minor style update

29b6de5

Updated migrated string to reference to migration store

57b1bf5

michael-wb mentioned this pull request Apr 5, 2023

Perform client reset when the client migrates between PBS and FLX #6354

Merged

3 tasks

Fix 'app destroyed during token refresh' test

2f70ed9

kmorkos reviewed Apr 6, 2023

View reviewed changes

src/realm/sync/noinst/client_impl_base.cpp Outdated Show resolved Hide resolved

src/realm/sync/noinst/migration_store.cpp Show resolved Hide resolved

Michael Wilkerson-Barker added 3 commits April 6, 2023 15:13

Merge branch 'master' of github.com:realm/realm-core into mwb/bind-ms…

a393310

…g-updates Adjusted saving of migrated partition based on new changes

Merge branch 'master' of github.com:realm/realm-core into mwb/bind-ms…

0c52615

…g-updates

Added debug message for sent BIND messages; clang-format

54bcc1b

michael-wb requested review from jbreams and kmorkos April 6, 2023 20:09

jbreams reviewed Apr 6, 2023

View reviewed changes

Michael Wilkerson-Barker added 2 commits April 6, 2023 17:46

Updates from review

a8aba8c

Merge branch 'master' of github.com:realm/realm-core into mwb/bind-ms…

1a0593c

…g-updates

michael-wb requested a review from jbreams April 11, 2023 16:17

jbreams approved these changes Apr 11, 2023

View reviewed changes

Updated changelog; renamed columns for migration store table

944d91b

kmorkos approved these changes Apr 11, 2023

View reviewed changes

src/realm/sync/noinst/migration_store.cpp Outdated Show resolved Hide resolved

Updated RevertToPBS handling to throw exception if client natively us…

e72d185

…ing FLX

Michael Wilkerson-Barker added 3 commits April 11, 2023 18:36

Added InvalidServerResponse error code

3224b0a

Removed comment

aebfdd1

string_to_error_code list must be in alphabetical order

84eed56

danieltabacaru reviewed Apr 12, 2023

View reviewed changes

danieltabacaru approved these changes Apr 12, 2023

View reviewed changes

Updates from review

9fb8740

michael-wb merged commit be4192d into master Apr 12, 2023

michael-wb deleted the mwb/bind-msg-updates branch April 12, 2023 14:30

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

Add migrated state information to flexible sync client BIND message #6464

Add migrated state information to flexible sync client BIND message #6464

Conversation

michael-wb commented Mar 31, 2023 • 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

danieltabacaru commented Apr 4, 2023

michael-wb commented Apr 4, 2023

jbreams 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

jbreams 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

michael-wb commented Mar 31, 2023 •

edited

Loading