Update BIND message with reason on what a session is used for #6902
Conversation
There was a problem hiding this comment.
can we hold off on the protocol bump until #6476 is merged? (assuming the timeframe for that is no more than a few days cc @michael-wb)
| @@ -52,6 +52,17 @@ std::string ClientResetOperation::get_fresh_path_for(const std::string& path) | |||
| return path + fresh_suffix; | |||
| } | |||
|
|
|||
| bool ClientResetOperation::is_fresh_path(const std::string& path) | |||
| { | |||
| const std::string fresh_suffix = ".fresh"; | |||
There was a problem hiding this comment.
can we add this as a const somewhere so we don't risk it going out of sync with get_fresh_path_for ?
| session_config.session_reason = ClientResetOperation::is_fresh_path(m_config.path) | ||
| ? sync::SessionReason::ClientReset | ||
| : sync::SessionReason::Sync; |
There was a problem hiding this comment.
Is there any other way to tell if it's a client reset other than the path?
Perhaps you could set a "client reset in progress value" in the SyncSession created in download_fresh_realm()?
There was a problem hiding this comment.
We could do that, but we depend on when the session becomes active and sync::Session is created (we need to have the value set before that, otherwise it's too late). Ideally, this value would be set/inferred in the constructor of the SyncSession, but we need to pass the value through several layers. I can make the change if you feel strongly about it.
There was a problem hiding this comment.
OK - given our current session structure, looking for the path suffix seems like the most efficient approach, and get_fresh_path_for() is only used during download_fresh_realm().
| @@ -1852,6 +1852,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["sessionReason"] = static_cast<uint64_t>(get_session_reason()); | |||
There was a problem hiding this comment.
@kmorkos - should this be a string value, since most of our protocol "enum" values are strings instead of ints?
There was a problem hiding this comment.
i have no strong preference. thoughts @yaseenmongo?
regardless, it would be nice to represent a "normal" session as the zero-value of whatever type we choose for backwards compatibility
There was a problem hiding this comment.
A normal session has a value of zero indeed
There was a problem hiding this comment.
I don't have a preference either. Since this is a wire protocol detail, I would generally say an int is better just for the slight efficiency gain, but it's not that much data to really matter. I'll defer to you folks 🙂
There was a problem hiding this comment.
it is an integer then 🙂
There was a problem hiding this comment.
One note, one potential benefit of using an int is that we could use convert it as a bitmask in the future if we ever needed to communicate multiple reasons why a client is reconnecting to sync e.g. ClientReset and SomeOtherReason. But I'm not sure if that will ever become a requirement.
we'll do that. |
What, How & Why?
The sync client relies on the server sending the schema instructions in case of a client reset. But as part of "Allow breaking changes in dev mode" project the server waits to receive the client schema before sending its schema. When dealing with a client reset the client never sends its schema and therefore it does not receive the server schema anymore. This PR introduces a fix so the server can differentiate if a synchronization session is used for a client reset or not, so it can send its schema.
The version of the sync protocol was bumped to 10.
☑️ ToDos
[ ] 🚦 Tests (or not relevant)[ ] C-API, if public C++ API changed.