Skip to content
This repository has been archived by the owner on Nov 8, 2021. It is now read-only.

Add support for new client reset functionality #778

Merged
merged 15 commits into from
Aug 26, 2019
Merged

Conversation

tgoyne
Copy link
Member

@tgoyne tgoyne commented Mar 9, 2019

Currently actually just async open.

@tgoyne tgoyne self-assigned this Mar 9, 2019
@bmunkholm bmunkholm added this to the 2019-w10 milestone Mar 12, 2019
@realm-ci realm-ci force-pushed the tg/client-reset branch 2 times, most recently from 6c9917e to 3a1ab90 Compare March 14, 2019 21:57
@geragray geragray removed this from the 2019-w10 milestone Mar 19, 2019
@bmunkholm bmunkholm added this to the 2019-w12 milestone Mar 19, 2019
@bmunkholm bmunkholm changed the title Add support for new async open/client reset functionality Add support for new async open functionality Mar 19, 2019
@bmunkholm
Copy link
Contributor

@tgoyne Is this ready or is anything missing?
I assume we still keep this branched to support OS release without it?

@bmunkholm
Copy link
Contributor

Status: Works as intended for Cocoa needs. Likely needs additional changes to support other SDKs. @cmelchior

@tgoyne tgoyne force-pushed the tg/client-reset branch 4 times, most recently from 31d287f to eeb51ee Compare April 26, 2019 20:46
@tgoyne tgoyne changed the title Add support for new async open functionality Add support for new client reset functionality Apr 26, 2019
@tgoyne tgoyne changed the base branch from master to sync-4.x April 26, 2019 20:47
@tgoyne tgoyne mentioned this pull request Apr 26, 2019
@bmunkholm bmunkholm assigned cmelchior and nirinchev and unassigned tgoyne Apr 30, 2019
@tgoyne tgoyne changed the base branch from sync-4.x to master August 22, 2019 19:37
@tgoyne tgoyne marked this pull request as ready for review August 22, 2019 19:37
@tgoyne
Copy link
Member Author

tgoyne commented Aug 22, 2019

This is now up-to-date with latest master/core/sync. IIRC there were more things I wanted to test related to it, but nothing that would actually block merging this.

Dockerfile Outdated Show resolved Hide resolved
@@ -181,9 +183,7 @@ stage('unit-tests') {

stage('publish') {
node('docker') {
publishReport('linux')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove these?

Copy link
Member Author

Choose a reason for hiding this comment

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

Building with coverage enabled greatly increases build time, and because we don't try to test any of the non-sync fallback code paths, the non-sync coverage reports don't tell us anything useful that isn't already covered by the sync-enabled coverage reports.

std::map<std::string, std::string> custom_http_headers;

util::Optional<std::string> url_prefix = none;

// The name of the directory which Realms should be backed up to following
// a client reset
util::Optional<std::string> recovery_directory = none;
util::Optional<std::string> recovery_directory;
ClientResyncMode client_resync_mode = ClientResyncMode::Recover;
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no doubt that this is the sensible default, but should it be considered a breaking change for the SDK's? In the strictest sense, I think it is, but for almost all practical purposes this is better.

In any case, the bindings can override this to get the default behavior they want.

Copy link
Member Author

Choose a reason for hiding this comment

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

The argument was that we're turning an error scenario into a non-error rather than changing the behavior of correct things. The assumption is that in normal usage when running against a perfect server with infinite resource a client reset error will never occur, so client resync just lets us pretend that we're in that scenario more often than we actually are.

src/sync/sync_session.cpp Outdated Show resolved Hide resolved
src/sync/sync_session.hpp Outdated Show resolved Hide resolved
@tgoyne tgoyne merged commit 2786752 into master Aug 26, 2019
@tgoyne tgoyne deleted the tg/client-reset branch August 26, 2019 20:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants