Guarantee that bootstraps get applied if they receive the final bootstrap message #5331

jbreams · 2022-03-16T21:26:19Z

What, How & Why?

When FLX sync is enabled we need to defer applying download messages that contain partial bootstrap changesets until we have the entire set of changesets available. This is because if the bootstrap is interrupted, when we retry the bootstrap later on, the server may be using a different snapshot with different results than the first bootstrap attempt. By using a different snapshot, the server may have a different set of objects that are in-view between bootstrap attempts, and if we were to apply partial bootstrap download messages, those objects that were in-view in the first attempt but out-of-view in the second attempt will be orphaned on the client.

This change introduces a new PendingBootstrapStore that will store the contents of bootstrap download messages in a side-table until a download message with last_in_batch=true has been received. If the bootstrap is interrupted before a last_in_batch=true message is received, none of the prior messages shall be applied and the bootstrap will restart. If the bootstrap is interrupted after the last_in_batch=true message is received, but before the bootstrap has been fully applied, the sync client shall apply all the remaining bootstrap messages before sending the IDENT message an resuming the session.

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)

danieltabacaru · 2022-03-22T09:22:20Z

src/realm/sync/client.cpp

+                VersionInfo version_info;
+                ClientReplication& repl = access_realm(); // Throws
+                if (m_wrapper.m_flx_pending_bootstrap_store) {
+                    while (m_wrapper.m_flx_pending_bootstrap_store->has_pending()) {


will we have the same problem if we don't get to apply all changeset batches here? I thought the idea was to apply all or nothing (i.e., concatenate all batches and have one single call to integrate_changesets)

jbreams · 2022-05-18T23:41:30Z

@danieltabacaru and @ironage , do you think you'll have time to review this soon?

danieltabacaru · 2022-05-19T10:23:37Z

src/realm/sync/noinst/pending_bootstrap_store.hpp

+    // state.
+    PendingBatch peek_pending(size_t limit_in_bytes);
+
+    // Removes the first set of changesets from the current pending bootstrap batch. The transaction must be int the


danieltabacaru · 2022-05-19T10:23:37Z

src/realm/sync/noinst/pending_bootstrap_store.hpp

+    // state.
+    PendingBatch peek_pending(size_t limit_in_bytes);
+
+    // Removes the first set of changesets from the current pending bootstrap batch. The transaction must be int the


danieltabacaru · 2022-05-19T10:24:38Z

src/realm/sync/noinst/pending_bootstrap_store.hpp

+    // writing state.
+    void pop_front_pending(const TransactionRef& tr, size_t count);
+
+    // Adds a set of changesets to the store and returns the local version produced by the transaction.


does not return anything

danieltabacaru · 2022-05-19T10:24:38Z

src/realm/sync/noinst/pending_bootstrap_store.hpp

+    // writing state.
+    void pop_front_pending(const TransactionRef& tr, size_t count);
+
+    // Adds a set of changesets to the store and returns the local version produced by the transaction.


does not return anything

danieltabacaru · 2022-05-19T11:14:09Z

src/realm/sync/noinst/pending_bootstrap_store.cpp

+    // If there are any incomplete bootstraps in the table, we need to clear them and then update our has_pending
+    // flag accordingly.
+    if (auto bootstrap_table = tr->get_table(m_table); !bootstrap_table->is_empty()) {
+        m_has_pending = (bootstrap_table->is_empty() == false);


doesn't the check above conclude that bootstrap_table is not empty?

Fixed. also removed the comment above since that gets handled elsewhere now.

danieltabacaru · 2022-05-19T11:14:09Z

src/realm/sync/noinst/pending_bootstrap_store.cpp

+    // If there are any incomplete bootstraps in the table, we need to clear them and then update our has_pending
+    // flag accordingly.
+    if (auto bootstrap_table = tr->get_table(m_table); !bootstrap_table->is_empty()) {
+        m_has_pending = (bootstrap_table->is_empty() == false);


doesn't the check above conclude that bootstrap_table is not empty?

danieltabacaru · 2022-05-19T11:48:18Z

src/realm/sync/noinst/pending_bootstrap_store.cpp

+        ret.changeset_data.push_back(util::AppendBuffer<char>());
+        auto& uncompressed_buffer = ret.changeset_data.back();
+
+        auto raw_changeset_data = cur_changeset.get<BinaryData>(m_changeset_data);


nit: you could name this compressed_changeset_data

danieltabacaru · 2022-05-19T11:48:18Z

src/realm/sync/noinst/pending_bootstrap_store.cpp

+        ret.changeset_data.push_back(util::AppendBuffer<char>());
+        auto& uncompressed_buffer = ret.changeset_data.back();
+
+        auto raw_changeset_data = cur_changeset.get<BinaryData>(m_changeset_data);


nit: you could name this compressed_changeset_data

danieltabacaru · 2022-05-19T11:53:17Z

src/realm/sync/noinst/pending_bootstrap_store.cpp

+                        bootstrap_obj.get<int64_t>(m_query_version), changeset_list.size());
+    }
+
+    m_has_pending = (bootstrap_table->is_empty() == false);


why not m_has_pending = !bootstrap_table->is_empty()?

danieltabacaru · 2022-05-19T11:53:17Z

src/realm/sync/noinst/pending_bootstrap_store.cpp

+                        bootstrap_obj.get<int64_t>(m_query_version), changeset_list.size());
+    }
+
+    m_has_pending = (bootstrap_table->is_empty() == false);


why not m_has_pending = !bootstrap_table->is_empty()?

danieltabacaru · 2022-05-19T12:38:40Z

src/realm/sync/client.hpp

-        std::function<void()> on_download_message_received_hook;
+        std::function<void(const sync::SyncProgress&, int64_t, sync::DownloadBatchState)>
+            on_download_message_received_hook;
+        /// Will be called after each download message is integrated. For use in testing.


nit: this is a bit misleading since we do not guarantee this is called for each download messages received (i.e., we "construct" our own download messages through peek_pending)
Edit: This actually seems to be called when a download message is added to the store.

I've copied the comment in src/realm/sync/config.hpp for this parameter here since I think that one is actually accurate.

danieltabacaru · 2022-05-19T12:38:40Z

src/realm/sync/client.hpp

-        std::function<void()> on_download_message_received_hook;
+        std::function<void(const sync::SyncProgress&, int64_t, sync::DownloadBatchState)>
+            on_download_message_received_hook;
+        /// Will be called after each download message is integrated. For use in testing.


nit: this is a bit misleading since we do not guarantee this is called for each download messages received (i.e., we "construct" our own download messages through peek_pending)

danieltabacaru · 2022-05-19T12:52:16Z

src/realm/sync/noinst/client_impl_base.cpp


-    if (batch_state == DownloadBatchState::LastInBatch) {
-        update_progress(progress); // Throws
+    if (process_flx_bootstrap_message(progress, batch_state, query_version, received_changesets)) {


shouldn't be pass downloadable_bytes and save it in the metadata table too?

downloadable_bytes doesn't really have meaning in FLX sync and this should always be zero.

danieltabacaru · 2022-05-19T12:52:16Z

src/realm/sync/noinst/client_impl_base.cpp


-    if (batch_state == DownloadBatchState::LastInBatch) {
-        update_progress(progress); // Throws
+    if (process_flx_bootstrap_message(progress, batch_state, query_version, received_changesets)) {


shouldn't be pass downloadable_bytes and save it in the metadata table too?

danieltabacaru · 2022-05-19T12:58:31Z

src/realm/sync/config.hpp

-    std::function<void(std::weak_ptr<SyncSession>)> on_download_message_received_hook;
+    std::function<void(std::weak_ptr<SyncSession>, const sync::SyncProgress&, int64_t, sync::DownloadBatchState)>
+        on_download_message_received_hook;
+    // Will be called after each bootstrap message is added to the pending bootstrap store, but before_frozen


this comment is a bit more accurate, but I'm not sure what before_frozen has to do with it.

I think my new auto-completer in neovim is auto-completing some nonsense.

danieltabacaru · 2022-05-19T12:58:31Z

src/realm/sync/config.hpp

-    std::function<void(std::weak_ptr<SyncSession>)> on_download_message_received_hook;
+    std::function<void(std::weak_ptr<SyncSession>, const sync::SyncProgress&, int64_t, sync::DownloadBatchState)>
+        on_download_message_received_hook;
+    // Will be called after each bootstrap message is added to the pending bootstrap store, but before_frozen


this comment is a bit more accurate, but I'm not sure what before_frozen has to do with it.

danieltabacaru · 2022-05-19T13:00:24Z

src/realm/sync/noinst/client_impl_base.hpp

+    bool process_flx_bootstrap_message(const SyncProgress& progress, DownloadBatchState batch_state,
+                                       int64_t query_version, const ReceivedChangesets& received_changesets);
+
+    // Processes any pending FLX bootstraps, if one excists. Otherwise this is a noop.


danieltabacaru · 2022-05-19T13:00:24Z

src/realm/sync/noinst/client_impl_base.hpp

+    bool process_flx_bootstrap_message(const SyncProgress& progress, DownloadBatchState batch_state,
+                                       int64_t query_version, const ReceivedChangesets& received_changesets);
+
+    // Processes any pending FLX bootstraps, if one excists. Otherwise this is a noop.


danieltabacaru · 2022-05-19T13:01:42Z

src/realm/sync/noinst/sync_metadata_schema.hpp

@@ -34,6 +34,8 @@ using TransactionRef = std::shared_ptr<Transaction>;
 namespace realm::sync {
 namespace internal_schema_groups {
 constexpr static std::string_view c_flx_subscription_store("flx_subscription_store");
+constexpr static std::string_view c_pending_bootstraps("PendingBootstraps");
+


please remove

danieltabacaru · 2022-05-19T13:01:42Z

src/realm/sync/noinst/sync_metadata_schema.hpp

@@ -34,6 +34,8 @@ using TransactionRef = std::shared_ptr<Transaction>;
 namespace realm::sync {
 namespace internal_schema_groups {
 constexpr static std::string_view c_flx_subscription_store("flx_subscription_store");
+constexpr static std::string_view c_pending_bootstraps("PendingBootstraps");
+


please remove

ironage

Looks good to me

ironage · 2022-05-19T17:24:01Z

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

@@ -19,6 +19,7 @@
 #ifndef REALM_OS_SYNC_SESSION_HPP
 #define REALM_OS_SYNC_SESSION_HPP

+#include "realm/sync/client_base.hpp"


is this necessary?

no. I think this got accidentally added by clangd

ironage · 2022-05-19T17:55:08Z

src/realm/sync/noinst/pending_bootstrap_store.cpp

+
+    auto tr = m_db->start_write();
+    auto bootstrap_table = tr->get_table(m_table);
+    auto incomplete_bootstraps = Query(bootstrap_table).less(m_query_version, query_version).find_all();


Only one bootstrap can be active at a time right? Can we change less to not_equal? I haven't implemented this yet, but I think the query version may decrease in a client reset with recovery. Or actually I may need to just clear out the entire PendingBootstrap store upon reset.

hmmmmm... I think we need to guarantee that the query version doesn't decrease? or at least we may need to coordinate that with the server. I think the server currently assumes that the query version will never decrease over the life of a file ident.

but yes, we can change this to not_equal.

ironage · 2022-05-19T18:02:42Z

src/realm/sync/noinst/pending_bootstrap_store.cpp

+}
+
+void PendingBootstrapStore::pop_front_pending(const TransactionRef& tr, size_t count)
+{


assert that tr is a write transaction?

ironage · 2022-05-19T18:07:31Z

src/realm/sync/noinst/sync_metadata_schema.hpp

@@ -34,6 +34,8 @@ using TransactionRef = std::shared_ptr<Transaction>;
 namespace realm::sync {
 namespace internal_schema_groups {
 constexpr static std::string_view c_flx_subscription_store("flx_subscription_store");
+constexpr static std::string_view c_pending_bootstraps("PendingBootstraps");


are we doing snake_case or CamelCaps?

received.

cla-bot bot added the cla: yes label Mar 16, 2022

danieltabacaru reviewed Mar 22, 2022

View reviewed changes

jbreams force-pushed the jbr/batch_up_bootstrap branch 3 times, most recently from 59d7b4c to 986e35d Compare May 6, 2022 17:35

jbreams force-pushed the jbr/batch_up_bootstrap branch from a41ac48 to 765f8ac Compare May 11, 2022 18:46

jbreams changed the title ~~Jbr/batch up bootstrap~~ Guarantee that bootstraps get applied if they receive the final bootstrap message May 11, 2022

jbreams marked this pull request as ready for review May 11, 2022 19:12

jbreams requested review from danieltabacaru and ironage May 11, 2022 19:12

danieltabacaru reviewed May 19, 2022

View reviewed changes

danieltabacaru approved these changes May 19, 2022

View reviewed changes

ironage approved these changes May 19, 2022

View reviewed changes

jbreams force-pushed the jbr/batch_up_bootstrap branch 3 times, most recently from 104ff16 to 4cefac0 Compare May 23, 2022 17:20

jbreams mentioned this pull request May 24, 2022

Ensure ChangesetIndex ranges are sorted by version and order within download message #5527

Merged

2 tasks

jbreams and others added 8 commits May 24, 2022 15:51

Guarantee that bootstraps will be applied if the full bootstrap is

cc85e4b

received.

fixups

fcdcf1d

more fixups

4f8573b

fixes from PR

711b1ed

format

359b187

changelog and fix test hang

20bdfd6

package.swift update

f717cd2

fix up stuff

1266fc3

jbreams force-pushed the jbr/batch_up_bootstrap branch from 0d2740b to 1266fc3 Compare May 24, 2022 19:52

jbreams and others added 4 commits May 24, 2022 16:23

fix typo

daa2afb

fix other typo

cc6bc8f

fix warning

63bdb96

fix tsan warnings

1829538

jbreams merged commit 5f7fac2 into master May 25, 2022

jbreams deleted the jbr/batch_up_bootstrap branch May 25, 2022 03:40

jbreams mentioned this pull request Jun 1, 2022

Update FLX subscription state after reporting sync progress #5553

Merged

3 tasks

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

Guarantee that bootstraps get applied if they receive the final bootstrap message #5331

Guarantee that bootstraps get applied if they receive the final bootstrap message #5331

Conversation

jbreams commented Mar 16, 2022 • edited Loading

What, How & Why?

☑️ ToDos

danieltabacaru Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

jbreams commented May 18, 2022

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 May 19, 2022 • edited Loading

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

ironage 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

jbreams commented Mar 16, 2022 •

edited

Loading

danieltabacaru Mar 22, 2022 •

edited

Loading

danieltabacaru May 19, 2022 •

edited

Loading