-
Notifications
You must be signed in to change notification settings - Fork 170
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.
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
RCORE-1872 Sync client should allow server bootstrapping at any time #7440
Changes from 26 commits
d09e4a3
ae2574e
43391ca
35b1483
3424431
e0d6ac1
f3eb7e0
7356ba2
9e32480
4431985
b87bc65
8447461
75a1d13
44d8e12
944fb30
a9807d3
c0b1a49
60cb61e
37acf98
8d9557d
400aeba
4924597
5ea89e2
3ad02b6
06bdc01
3198ebd
7902db0
fe86341
7a5269b
0a26a71
c7a4739
2355dd9
509c97b
8a00447
b7b7180
484f526
f29542b
4f09342
3c193ec
4a4f13f
a497259
8ecfe34
f1d9425
e375bfd
c4f0344
2aa76d9
39af9d6
165a651
a72e158
b850cac
e2cd353
9c7b7af
2fcdbec
83d2a5c
5f2a7e1
51b54eb
93386d4
7e2c5a7
b2fcf51
239d162
841f1d8
0f00b92
1ffddd0
04624e9
84a4c23
3e97840
c4e6a9e
3649284
aede51b
0d55022
a40bafc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -839,12 +839,8 @@ void SessionImpl::update_subscription_version_info() | |
bool SessionImpl::process_flx_bootstrap_message(const SyncProgress& progress, DownloadBatchState batch_state, | ||
int64_t query_version, const ReceivedChangesets& received_changesets) | ||
{ | ||
// Ignore the call if the session is not active | ||
if (m_state != State::Active) { | ||
return false; | ||
} | ||
|
||
if (is_steady_state_download_message(batch_state, query_version)) { | ||
// Ignore the message if the session is not active or a steady state message | ||
if (m_state != State::Active || batch_state == DownloadBatchState::SteadyState) { | ||
return false; | ||
} | ||
|
||
|
@@ -1103,24 +1099,53 @@ SyncClientHookAction SessionImpl::call_debug_hook(SyncClientHookEvent event, con | |
return call_debug_hook(data); | ||
} | ||
|
||
bool SessionImpl::is_steady_state_download_message(DownloadBatchState batch_state, int64_t query_version) | ||
bool SessionImpl::is_steady_state_download_message(DownloadBatchState batch_state, int64_t query_version, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pretty convoluted. I have an approach bellow where this method should not be needed at all anymore. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jbreams had some suggestions on a separate commit that reworked this function and the tracking of the changeset remote versions. This has been integrated into this PR and the tests have been updated to validate the different paths depending on how the changeset looks. |
||
bool same_remote_versions) | ||
{ | ||
// Should never be called if session is not active | ||
REALM_ASSERT_EX(m_state == State::Active, m_state); | ||
if (batch_state == DownloadBatchState::SteadyState) { | ||
return true; | ||
// Query version should always be the same or increasing | ||
REALM_ASSERT_3(query_version, >=, m_wrapper.m_flx_active_version); | ||
|
||
// Return early if already steady state or PBS (doesn't use bootstraps) | ||
if (batch_state == DownloadBatchState::SteadyState || !m_is_flx_sync_session) { | ||
return true; // Steady state | ||
} | ||
|
||
if (!m_is_flx_sync_session) { | ||
return true; | ||
// Bootstrap messages (i.e. non-steady state) are identified by: | ||
// * DownloadBatchState of MoreToCome | ||
// * DownloadBatchState of LastInBatch, and | ||
// * first LastInBatch=true after one or more MoreToCome messages | ||
// * query_version greater than the active query_version | ||
// * LastInBatch=true message has 2 or more changesets and all have the same | ||
// remote_version | ||
|
||
// LastInBatch=False messages are always a bootstrap message | ||
if (batch_state == DownloadBatchState::MoreToCome) { | ||
return false; // Not steady state | ||
} | ||
|
||
// If this is a steady state DOWNLOAD, no need for special handling. | ||
if (batch_state == DownloadBatchState::LastInBatch && query_version == m_wrapper.m_flx_active_version) { | ||
return true; | ||
// Messages with query_version greater than the active version are always a | ||
// bootstrap message | ||
if (query_version > m_wrapper.m_flx_active_version) { | ||
return false; // Not steady state | ||
} | ||
|
||
// If this is the first LastInBatch=true message after one or more | ||
// LastInBatch=false messages, this is the end of a bootstrap | ||
if (m_last_download_batch_state == DownloadBatchState::MoreToCome) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i wish we didn't have to add this state tracking, but I think I see why we need it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah - it's so we can catch the LastInBatch=false => LastInBatch=true sequence for the server initiated bootstraps. Can you think of a better way? |
||
return false; // Not steady state | ||
} | ||
|
||
// Otherwise, if this is a LastInBatch=true message whose query version matches | ||
// the current active version and all the changesets in the message have the | ||
// same remote version, then this is a server initiated single message bootstrap | ||
if (same_remote_versions) { | ||
return false; // Not steady state | ||
} | ||
|
||
return false; | ||
// If none of the previous checks were successful, then this is a steady state msg | ||
return true; // Steady state | ||
} | ||
|
||
void SessionImpl::init_progress_handler() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1891,7 +1891,7 @@ void Session::send_bind_message() | |
session_ident_type session_ident = m_ident; | ||
bool need_client_file_ident = !have_client_file_ident(); | ||
const bool is_subserver = false; | ||
|
||
m_last_download_batch_state = DownloadBatchState::SteadyState; | ||
|
||
ClientProtocol& protocol = m_conn.get_client_protocol(); | ||
int protocol_version = m_conn.get_negotiated_protocol_version(); | ||
|
@@ -2373,8 +2373,6 @@ Status Session::receive_download_message(const DownloadMessage& message) | |
// If this is a PBS connection, then every download message is its own complete batch. | ||
bool last_in_batch = is_flx ? *message.last_in_batch : true; | ||
auto batch_state = last_in_batch ? sync::DownloadBatchState::LastInBatch : sync::DownloadBatchState::MoreToCome; | ||
if (is_steady_state_download_message(batch_state, query_version)) | ||
batch_state = DownloadBatchState::SteadyState; | ||
|
||
auto&& progress = message.progress; | ||
if (is_flx) { | ||
|
@@ -2414,21 +2412,39 @@ Status Session::receive_download_message(const DownloadMessage& message) | |
return status; | ||
} | ||
|
||
version_type server_version = m_progress.download.server_version; | ||
// Start with the download server version from the last download message, since the changesets in the new | ||
// download message must have a remote version greater than (or equal to for FLX) this value. | ||
version_type last_remote_version = m_progress.download.server_version; | ||
version_type last_integrated_client_version = m_progress.download.last_integrated_client_version; | ||
// If there are 2 or more changesets in this message, track whether or not the changesets all have the same | ||
// download_server_version to help with determining if this is a single message server-initiated bootstrap or not. | ||
bool same_remote_version = last_in_batch && message.changesets.size() > 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if last_in_batch is false, then same_remote_version is false too and its value never changes. This is incorrect for multi-message bootstraps (where you rely on batch_state). I think this value should always reflect the reality. On another note, computing same_remote_version seems over-complicated. I think something like this should work: where |
||
bool after_first_changeset = false; | ||
|
||
for (const RemoteChangeset& changeset : message.changesets) { | ||
// Check that per-changeset server version is strictly increasing, except in FLX sync where the server | ||
// version must be increasing, but can stay the same during bootstraps. | ||
bool good_server_version = m_is_flx_sync_session ? (changeset.remote_version >= server_version) | ||
: (changeset.remote_version > server_version); | ||
// Check that per-changeset server version is strictly increasing since the last download server version, | ||
// except in FLX sync where the server version must be increasing, but can stay the same during bootstraps. | ||
bool good_server_version = m_is_flx_sync_session ? (changeset.remote_version >= last_remote_version) | ||
: (changeset.remote_version > last_remote_version); | ||
// Each server version cannot be greater than the one in the header of the download message. | ||
good_server_version = good_server_version && (changeset.remote_version <= progress.download.server_version); | ||
if (!good_server_version) { | ||
return {ErrorCodes::SyncProtocolInvariantFailed, | ||
util::format("Bad server version in changeset header (DOWNLOAD) (%1, %2, %3)", | ||
changeset.remote_version, server_version, progress.download.server_version)}; | ||
changeset.remote_version, last_remote_version, progress.download.server_version)}; | ||
} | ||
// Check to see if all the changesets in this LastInBatch=true message have the same remote_version | ||
// If so, this is a server-initiated single message bootstrap | ||
if (same_remote_version && after_first_changeset) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you decide to go with the current approach, then the following is enough: |
||
// After the first changeset compare the previous changeset's server version to the current changeset | ||
// server version. If they are different then this is definitely not a bootstrap message | ||
same_remote_version = changeset.remote_version == last_remote_version; | ||
} | ||
server_version = changeset.remote_version; | ||
// Skip the first changeset, since `server_version` contains the incorrect value for this check | ||
after_first_changeset = true; | ||
// Save the remote version for the current changeset to compare against the next changeset | ||
last_remote_version = changeset.remote_version; | ||
|
||
// Check that per-changeset last integrated client version is "weakly" | ||
// increasing. | ||
bool good_client_version = | ||
|
@@ -2453,6 +2469,11 @@ Status Session::receive_download_message(const DownloadMessage& message) | |
} | ||
} | ||
|
||
if (is_steady_state_download_message(batch_state, query_version, same_remote_version)) { | ||
batch_state = DownloadBatchState::SteadyState; | ||
} | ||
m_last_download_batch_state = batch_state; | ||
|
||
auto hook_action = call_debug_hook(SyncClientHookEvent::DownloadMessageReceived, progress, query_version, | ||
batch_state, message.changesets.size()); | ||
if (hook_action == SyncClientHookAction::EarlyReturn) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1133,6 +1133,10 @@ class ClientImpl::Session { | |
// the detection of download completion. | ||
request_ident_type m_last_triggering_download_mark = 0; | ||
|
||
// Keep track of the DownloadBatchState for the last download message to help | ||
// determine if the current LastInBatch came after a message with MoreToCome. | ||
DownloadBatchState m_last_download_batch_state = DownloadBatchState::SteadyState; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this is to distinguish between a LastInBatch meant as last message in a bootstrap and the one meant as SteadyState right? Can't the server send a third value for SteadyState? this would help with the one message bootstrap too and also simplify all the checks we currently do on the client. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wish they would do that, too - but they rejected the request when I asked towards the beginning of the project :( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't recall that coming up, or any specific reason that would have been unreasonable - do you remember any more context around that convo? It might have just gotten some pushback since it would require a protocol bump and those are always a bit of extra work, but I don't see a strict technical reason why it couldn't be done. Don't know if it makes sense at this point in the project, but we probably could add this on the server (unless I'm overlooking a concrete reason why we shouldn't) if it would provide a significant improvement here. |
||
|
||
SessionWrapper& m_wrapper; | ||
|
||
request_ident_type m_last_pending_test_command_ident = 0; | ||
|
@@ -1197,7 +1201,8 @@ class ClientImpl::Session { | |
SyncClientHookAction call_debug_hook(SyncClientHookEvent event, const ProtocolErrorInfo&); | ||
SyncClientHookAction call_debug_hook(const SyncClientHookData& data); | ||
|
||
bool is_steady_state_download_message(DownloadBatchState batch_state, int64_t query_version); | ||
bool is_steady_state_download_message(DownloadBatchState batch_state, int64_t query_version, | ||
bool same_remote_versions); | ||
|
||
void init_progress_handler(); | ||
void enable_progress_notifications(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,6 +57,11 @@ namespace sync { | |
// Server replaces 'downloadable_bytes' (which was always zero prior this version) | ||
// with an estimated progress value (double from 0.0 to 1.0) for flx sessions | ||
// | ||
// 13 Support for syncing collections (lists and dictionaries) in Mixed columns | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems it's going to be the other way around. Edit: not sure anymore 😅 |
||
// | ||
// 14 Support for server initiated bootstraps, including bootstraps for role/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you need to update There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was waiting until this feature was about to be merged, but it really doesn't matter, since I'm using a feature branch and the feature is behind a feature flag. |
||
// permissions changes instead of performing a client reset when changed. | ||
// | ||
// XX Changes: | ||
// - TBD | ||
// | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesn't look like this is used anywhere. do we need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, probably not - this was left over from when I was using the connection state to detect the reconnect, but that has been replaced by the error check in event hook. I'll remove it.