Skip to content

Add schema version to flexible sync client BIND message #6863

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

Merged
merged 3 commits into from
Aug 7, 2023
Merged

Add schema version to flexible sync client BIND message #6863

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
074a4b9
Send schema version as part of BIND messages
danieltabacaru Aug 4, 2023
2ee013f
Update comment
danieltabacaru Aug 7, 2023
768eafb
Changelog
danieltabacaru Aug 7, 2023
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
12 changes: 11 additions & 1 deletion src/realm/sync/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,8 @@ class SessionWrapper final : public util::AtomicRefCountBase, public SyncTransac
std::function<SyncClientHookAction(SyncClientHookData data)> m_debug_hook;
bool m_in_debug_hook = false;

const uint64_t m_schema_version;

std::shared_ptr<SubscriptionStore> m_flx_subscription_store;
int64_t m_flx_active_version = 0;
int64_t m_flx_last_seen_version = 0;
Expand Down Expand Up @@ -803,6 +805,13 @@ util::Optional<ClientReset>& SessionImpl::get_client_reset_config() noexcept
return m_wrapper.m_client_reset_config;
}

uint64_t SessionImpl::get_schema_version() noexcept
{
// Can only be called if the session is active or being activated
REALM_ASSERT_EX(m_state == State::Active || m_state == State::Unactivated, m_state);
return m_wrapper.m_schema_version;
}

void SessionImpl::initiate_integrate_changesets(std::uint_fast64_t downloadable_bytes, DownloadBatchState batch_state,
const SyncProgress& progress, const ReceivedChangesets& changesets)
{
Expand Down Expand Up @@ -1230,6 +1239,7 @@ SessionWrapper::SessionWrapper(ClientImpl& client, DBRef db, std::shared_ptr<Sub
, m_client_reset_config{std::move(config.client_reset_config)}
, m_proxy_config{config.proxy_config} // Throws
, m_debug_hook(std::move(config.on_sync_client_event_hook))
, m_schema_version(config.schema_version)
, m_flx_subscription_store(std::move(flx_sub_store))
, m_migration_store(std::move(migration_store))
{
Expand Down Expand Up @@ -2122,7 +2132,7 @@ void Client::voluntary_disconnect_all_connections()

bool Client::wait_for_session_terminations_or_client_stopped()
{
return m_impl.get()->wait_for_session_terminations_or_client_stopped();
return m_impl->wait_for_session_terminations_or_client_stopped();
}


Expand Down
3 changes: 3 additions & 0 deletions src/realm/sync/client.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,9 @@ class Session {
bool simulate_integration_error = false;

std::function<SyncClientHookAction(const SyncClientHookData&)> on_sync_client_event_hook;

/// Schema version
uint64_t schema_version = -1; // = ObjectStore::NotVersioned
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend updating the comment to indicate it's for FLX only, or update the value to flx_schema_version since this feature is only for FLX sync.

};

/// \brief Start a new session for the specified client-side Realm.
Expand Down
7 changes: 3 additions & 4 deletions src/realm/sync/noinst/client_impl_base.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,10 +204,8 @@ ClientImpl::ClientImpl(ClientConfig config)
REALM_ASSERT_EX(m_socket_provider, "Must provide socket provider in sync Client config");

if (m_one_connection_per_session) {
// FIXME: Re-enable this warning when the load balancer is able to handle
// multiplexing.
// logger.warn("Testing/debugging feature 'one connection per session' enabled - "
// "never do this in production");
logger.warn("Testing/debugging feature 'one connection per session' enabled - "
"never do this in production");
}

if (config.disable_upload_activation_delay) {
Expand Down Expand Up @@ -1861,6 +1859,7 @@ void Session::send_bind_message()
if (auto migrated_partition = get_migration_store()->get_migrated_partition()) {
bind_json_data["migratedPartition"] = *migrated_partition;
}
bind_json_data["schema_version"] = get_schema_version();
Copy link
Contributor

Choose a reason for hiding this comment

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

I am assuming the server will treat a -1 schema version the same as if the value was not provided.
Should we exclude the schema_version value if it is "not versioned"?
or maybe exclude it for now and always include it once we have tested core against the server schema versions feature?
If there are changes to core required to support the new feature, it could be used to advertise to the server that we support the feature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are client changes required for this feature. One way to advertise the client supports the feature is by bumping the sync protocol version (we're still debating if it's really needed). My thinking was that once an app is upgraded to support the future, the client will always report a schema version even if there is none.

We can always stop reporting the schema version in some cases if we deem it necessary (this is all going to a feature branch).

Copy link
Contributor

Choose a reason for hiding this comment

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

With the bind json data, I don't think you need to bump the proto version - hopefully, the presence of the value should be enough to indicate whether or not the client supports schema migrations (which was my concern if this was being merged to master). Although, this only works for FLX, and the proto version would need to be bumped if it also has to work for PBS.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, that's the idea. And that's why I think it would be a bit odd to sometimes not report it (that should only happen if the app is downgraded). But this may change once we start integration testing.

if (logger.would_log(util::Logger::Level::debug)) {
std::string json_data_dump;
if (!bind_json_data.empty()) {
Expand Down
3 changes: 3 additions & 0 deletions src/realm/sync/noinst/client_impl_base.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -927,6 +927,9 @@ class ClientImpl::Session {
// transfer from the server.
util::Optional<ClientReset>& get_client_reset_config() noexcept;

/// Returns the schema version the synchronization session connects with to the server.
uint64_t get_schema_version() noexcept;

/// \brief Initiate the integration of downloaded changesets.
///
/// This function must provide for the passed changesets (if any) to
Expand Down