RCORE-2007 Added Resumption delay configuration to SyncClientTimeouts #7441
Conversation
michael-wb
changed the title
Pull Request Test Coverage Report for Build michael.wilkersonbarker_985Details
💛 - Coveralls |
| @@ -74,6 +74,16 @@ struct SyncClient { | |||
| c.pong_keepalive_timeout = config.timeouts.pong_keepalive_timeout; | |||
| if (config.timeouts.fast_reconnect_limit > 1000) | |||
| c.fast_reconnect_limit = config.timeouts.fast_reconnect_limit; | |||
| if (config.timeouts.resumption_delay_interval >= 1000) | |||
There was a problem hiding this comment.
Why do we want to make the default values the floor rather than just the default? Tbh, I've always been skeptical of trying to force these arbitrary "sensible" ceilings. I'm def for threading this value down to the client but I'd be more in favor of just using whatever the client has passed in.
There was a problem hiding this comment.
Even though it's the default value, the floor values are meant to prevent spamming the server too much. I don't think it would be reasonable to have a bunch of clients trying to reconnect every 250ms with a max retry of 2 secs... I chose 1 second and 30 seconds as a reasonable tradeoff of allowing lower values without spamming the server too much.
tbh - I should use the floor values instead of the defaults if the passed in value is lower than the floor.
There was a problem hiding this comment.
Removed most of the checks - SyncClient will now throw an InvalidArgument exception if the multiplier is less than 1 or the divisor is less than 0. Also added an info debug that will print if the resumption delay interval is less than 1 second.
There was a problem hiding this comment.
Setting a minimum value was done at the request of the (pre-acquisition) server team. Some of our original users liked to set very stupid values for every single value we let them set and this resulted in a lot of wasted support effort when everything broke due to nonsensical settings.
The current server team wants the reconnect jitter to still exist, so exposing a setting to effectively turn it off seems questionable?
There was a problem hiding this comment.
I agree with requiring at least minimum values to avoid spamming the server too much. Should we make the minimum jitter 2 so it can't be disabled - or we could just force it to always be 4 and ignore the configured value.
On the other hand, perhaps it would be better to not make these values configurable to avoid the risk of breaking sync reconnects?
There was a problem hiding this comment.
We'd actually need a maximum value; smaller numbers increase the jitter. I think that field in particular doesn't make much sense to be adjustable.
There was a problem hiding this comment.
Thanks - updated the delay_jitter_divisor value so it is no longer configurable.
…rable-retry-timers
* master: update release note prepare v14.4.0 sets not allowed at storage level inside mixed (#7502) 🔄 Synced file(s) with realm/ci-actions (#7481) RCORE-1982 Opening realm with cached user while offline results in fatal error and session does not retry connection (#7469) RCORE-2008 Bump baas version (#7499) RCORE-2027: Setting log callback should not override existing log level hierarchy (#7494) Derive correct ubuntu version on linuxmint (#7471) Include nested path in 'OutOfBounds' error message (#7489) fix depth for nested collections set to 4 in debug mode (#7486) Set the minimum buffer size in Group::write() to be equal to the page size (#7492) Update the parent's content version when bumping it in a nested collection (#7470) release notes core v14.3.0 (#7482) RCORE-2007 Added Resumption delay configuration to SyncClientTimeouts (#7441) Improve performance of aggregate operations on empty dictionaries (#7418)
What, How & Why?
Added values to the
SyncClientTimeoutsfor configuring the SyncSession resumption delay. This work was pulled from the recommendations posted for PR #7365 and separated out into its own pull request since that design is being reworked.☑️ ToDos
bindgen/spec.yml, if public C++ API changed