STITCH-1954 Implement instance id and counter versioning scheme #68

adamchel · 2018-10-11T22:28:34Z

This is complete to the best of my knowledge and based on offline conversations I've had with Eric, but I threw in some comments where I'm still unsure if stuff needs to be done or if it should be delegated to future tickets.

The tests from 1950 (specifically testConfigure) are still flaky, but otherwise everything passes.

I'll keep an eye on evergreen as well.

passing tests, but no new tests add some question comments progress with removing committed versions incomplete server test changes

coveralls · 2018-10-11T23:30:17Z

Coverage increased (+4.4%) to 59.766% when pulling be063aa on adamchel:STITCH-1954-wip-rebased into a28ddad on mongodb:master.

jsflax

Looking good! One possible minor and some q's.

jsflax · 2018-10-12T09:05:06Z

...ain/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizer.java

+      // TODO(QUESTION FOR REVIEW): The testFrozenDocumentConfig test fails if I call this, but
+      // other tests pass. Shouldn't the receipt of this document indicate that this is the last
+      // known remote version of this document?
+      // docConfig.setLastKnownRemoteVersion(lastKnownRemoteVersion);


[q] Where does testFrozenDocumentConfig fail?

no longer relevant, worked it out with Eric and we now* have the correct behavior

jsflax · 2018-10-12T09:20:13Z

...ain/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizer.java

      final BsonValue documentId,
      final BsonDocument document,
-      final BsonValue atVersion,
+      final BsonDocument atVersion,


[q] Why are we passing this around as a BsonDocument? Seems it may be cleaner to use its concrete type here.

This is because the consumer of atVersion is only operating on the pure BSON of the version, whereas DocumentVersionInfo is an abstraction on top of that that we use for various other checks.

DocumentVersionInfo is allowed to represent an empty version, meaning consumers of atVersion would then have to check for those kinds of states. I think this is fine for now, especially since consumers of atVersion never convert back to a DocumentVersionInfo, and they don't have any use for its methods/functionality

jsflax · 2018-10-12T09:50:09Z

...ain/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizer.java

+    // old version, rather than the new version. Is this intentional? It seems like we should be
+    // setting the pending writes at the newly generated version. I'm confused though because if I
+    // use the new version here instead, tests fail because updates don't end up happening on a
+    // sync pass.


We need to have had a conflict where the local version is accepted to get to this point. However, I think atVersion is really a misnomer for lastKnownRemoteVersion, as seen in the CoreDocumentSynchronizationConfig. So at this point in the code, we are preparing the document with the next version, but the pending writes are still currently at the lastKnownRemoteVersion, which is the previous version, since we are not writing to remote yet.

On the next pass, in syncLocalToRemote (since we have resolved the prior conflict), our document will be written to the remote, and setPendingWritesComplete will be called with the version within the document, which is now the lastKnownRemoteVersion.

I'd like @edaniels to double check my logic here, but I believe this is the reasoning. By committing this version to the config, we've essentially told the system that the conflict has been resolved and that this is a locally generated change event. Via both this and the logicalT checks, it will prevent this change event from getting mistakenly consumed in a R2L pass.

I'm also thinking it could be nice to have this tested as a unit test.

discussed offline. this is correct

jsflax · 2018-10-12T10:06:39Z

.../java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DocumentVersionInfo.java

+    return versionValues != null;
+  }
+
+  private @Nonnull VersionValues getVersionValuesThrowIfNull() {


[minor] Using ThrowIfNull as a suffix here, and the exception thrown here, is arguably a bit of an anti-pattern. If this is an illegal state, we should throw that exception in the constructor– otherwise, it's on the user to call the #isNonEmptyVersion check.

jsflax · 2018-10-12T10:08:42Z

.../java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DocumentVersionInfo.java

+  ) {
+    if (version != null) {
+      this.versionDoc = version;
+      this.versionValues = new VersionValues(


[minor][related to comment below] We should be checking that these keys exist on the versionDoc before reading them.

If it the doc is null or they do not exist and this is a disallowed state, we should throw an IllegalStateException.

From what I can tell here, if the versionDoc is null, that should be an OK state, and is covered by isNonEmptyVersion. Maybe the exception should be if the versionDoc is malformed? Just food for thought– I think to an extent, there's a stylistic choice here.

Definitely agree that this class is a little funky, I refactored it a little bit per your comments and eric's comments to make it harder to reach an illegal state.

edaniels

Generally LGTM. There are tests to fix with the has committed writes.

edaniels · 2018-10-12T14:50:44Z

.../java/com/mongodb/stitch/android/services/mongodb/remote/internal/SyncMongoClientIntTests.kt

+
+        assertTrue(coll.syncedIds.contains(doc1Id))
+
+        // syncing on this document with an unsupported spv should cause the document to desync


[test] Check that we emitted an error for this

edaniels · 2018-10-12T14:52:17Z

...ain/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizer.java

              docConfig,
              remoteChangeEvent);
        } else {
+          // TODO(ERIC->ADAM): This event may contain old version info. We should be filtering out


Replace ERIC->ADAM with STITCH-1972

edaniels · 2018-10-12T14:52:49Z

.../java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DocumentVersionInfo.java

+    }
+  }
+
+  private static final String SYNC_PROTOCOL_VERSION_FIELD = "spv";


[nit] Move this into a Fields inner static class

edaniels · 2018-10-12T14:54:22Z

.../java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DocumentVersionInfo.java

+   * Returns whether this version is non-empty (i.e. a version from a document with no version)
+   * @return true if the version is non-empty, false if the version is empty.
+   */
+  boolean isNonEmptyVersion() {


[nit] easier to call this hasVersion

edaniels · 2018-10-12T14:54:50Z

.../java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DocumentVersionInfo.java

+    return versionValues != null;
+  }
+
+  private @Nonnull VersionValues getVersionValuesThrowIfNull() {


[nit] I would call this getVersion and make the class called a Version, not VersionValues

edaniels · 2018-10-12T14:55:10Z

.../java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DocumentVersionInfo.java

+    return this.versionValues;
+  }
+
+  /**


[nit] These methods should be on the version, not the class.

edaniels · 2018-10-12T15:11:44Z

...ain/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizer.java

+      // TODO(QFR): I ask because when we are looking at hasCommittedVersion, we are using the
+      // lastKnownRemoteVersion of the doc config rather than the last known remote version based
+      // on the incoming change event we got here, which would make this not conform to spec.
+      if (docConfig.hasCommittedVersion(localVersion)) {


[minor] This should just be remote version

edaniels · 2018-10-12T15:12:19Z

...ain/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizer.java

    final BsonDocument localDocument = localColl.find(docFilter).first();

-    final BsonValue lastKnownRemoteVersion;
+    final BsonDocument lastKnownRemoteVersion;


if it helps, name this currentRemoteVersion which will become lastKnownRemoteVersion in the config

edaniels · 2018-10-12T15:13:18Z

...ain/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizer.java

+        return;
+      }
+
+      // TODO(QUESTION FOR REVIEW): The testFrozenDocumentConfig test fails if I call this, but


There reason not to set this and why you can get rid of that set method is this would always make the next statement true.

edaniels · 2018-10-12T15:15:16Z

...ain/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizer.java

+    // old version, rather than the new version. Is this intentional? It seems like we should be
+    // setting the pending writes at the newly generated version. I'm confused though because if I
+    // use the new version here instead, tests fail because updates don't end up happening on a
+    // sync pass.


discussed offline. this is correct

adamchel · 2018-10-12T19:42:11Z

@edaniels @jsflax this is ready for another look. On top of resolving your feedback, I also did some general cleanup in DataSynchronizer.java to get rid of redundant code and code that was made unnecessary with some of the other refactors that we made

edaniels

LGTM. Just a unit test class to add

edaniels · 2018-10-12T19:44:55Z

.../java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DocumentVersionInfo.java

+   * empty version.
+   * @return a BsonDocument representing a synchronization version
+   */
+  BsonDocument getNextVersion() {


[nit] Add a unit test(s) for this

jsflax

LGTM 👍

jsflax · 2018-10-12T19:53:58Z

Could use a unit test for the logic we spoke about earlier in this PR.

adamchel added 6 commits October 10, 2018 19:02

initial progress (not tests)

6971f31

passing tests, but no new tests add some question comments progress with removing committed versions incomplete server test changes

eric patch

c53cd8b

add a working test for the versioning scheme

3b8e923

add test for unsupported spv, and clean up

f8ba543

fix checkstyle and findbugs

9957b78

self review and fix lint

33213d4

adamchel requested review from edaniels and jsflax October 11, 2018 22:28

jsflax suggested changes Oct 12, 2018

View reviewed changes

appease linter

7bc9744

edaniels suggested changes Oct 12, 2018

View reviewed changes

adamchel added 2 commits October 12, 2018 14:03

address PR comments

677de20

self review cleanup

be063aa

edaniels approved these changes Oct 12, 2018

View reviewed changes

jsflax approved these changes Oct 12, 2018

View reviewed changes

add unit test for DocumentVersionInfo

6f81bfe

adamchel merged commit d9d9d64 into mongodb:master Oct 12, 2018


		assertTrue(coll.syncedIds.contains(doc1Id))

		// syncing on this document with an unsupported spv should cause the document to desync

STITCH-1954 Implement instance id and counter versioning scheme #68

STITCH-1954 Implement instance id and counter versioning scheme #68

Uh oh!

Conversation

adamchel commented Oct 11, 2018

Uh oh!

coveralls commented Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsflax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamchel Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edaniels left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamchel commented Oct 12, 2018

Uh oh!

edaniels left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsflax left a comment

Choose a reason for hiding this comment

Uh oh!

jsflax commented Oct 12, 2018

Uh oh!

Uh oh!

coveralls commented Oct 11, 2018 •

edited

Loading

adamchel Oct 12, 2018 •

edited

Loading

edaniels left a comment •

edited

Loading