STITCH-1950 Add collection level sync configuration #67

jsflax · 2018-10-09T20:45:57Z

Notes: I've handled the possible null case here in what I felt was best for usability, however, it may need to be agreed upon.

Description:
Configuration for sync happens at collection.sync().configure() where configure is: configure(, , ). See spec for more info.

This configure should be the signal to the data synchronizer to start synchronzing if it hasn't already. Subsequent configurations just update handlers and listeners. The signal is to prevent an error condition where we were to sync but do not know how to handle conflcits since a handler has not been set up yet. Most app code will always be calling configure first.

jsflax · 2018-10-09T20:46:36Z

Seems like some formatting got included in this PR. I can remove it, or we can keep it if we want.

jsflax · 2018-10-10T12:07:21Z

Evg is being flakey. Otherwise this is ready for review. The driveby tab changes are valid, however, I can move them to another PR if we feel that it's here inappropriately.

edaniels · 2018-10-10T12:23:52Z

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

-                "mongodb",
-                "mongodb1",
-                ServiceConfigs.Mongo(getMongoDbUri()))
+            app.second,


Let's remove the reformats to avoid conflicts

edaniels

Done with first pass

edaniels · 2018-10-10T12:25:15Z

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

    );
-    this.triggerListeningToNamespace(namespace);
+
+    if (!this.isConfigured) {


[minor] We should grab the lock first before checking configured otherwise this can be a torn read or write.

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

edaniels · 2018-10-10T12:26:52Z

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

        (remoteEvent.getFullDocument() == null && resolvedDocument == null)
            || (remoteEvent.getFullDocument() != null
-            && transformedRemoteEvent.getFullDocument().equals(resolvedDocument));
+            && remoteEvent.getFullDocument().equals(resolvedDocument));


[major] This is reintroducing a bug. Please undo any reformats or changes unrelated to this ticket in this PR.

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

edaniels · 2018-10-10T12:29:48Z

...t/java/com/mongodb/stitch/server/services/mongodb/remote/internal/SyncMongoClientIntTests.kt

+        val docToInsert = Document("hello", "world")
+        testSync.insertOneAndSync(docToInsert)
+
+        assertFalse((mongoClient as RemoteMongoClientImpl).dataSynchronizer.isRunning)


[test] Instead of needing an isRunning as var and exposed, you can instead just add a semaphore to watch a change stream come in.

edaniels · 2018-10-10T12:31:41Z

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

+   *
+   * @return true if configured, false if not
+   */
+  boolean isConfigured() {


[minor] must be synchronized

edaniels · 2018-10-10T12:33:04Z

...t/java/com/mongodb/stitch/server/services/mongodb/remote/internal/SyncMongoClientIntTests.kt

+    fun testConfigure() {
+        val testSync = getTestSync()
+
+        (mongoClient as RemoteMongoClientImpl).dataSynchronizer.enableSyncThread()


I'm not quite positive, but it seems like the expose and implementation of .enableSyncThread() is not strictly necessary. You can use manual sync passes if you need to.

Alright. This makes more sense. It will slightly change the tests, but yes, if we address your comment below, we can remove the enableSyncThread method.

edaniels · 2018-10-10T12:33:38Z

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

    }
  }

+  public void enableSyncThread() {


[nit] Think this can be deleted.

Having this here allows us to check that the thread is started from within our tests.

edaniels · 2018-10-10T12:34:45Z

...tch/core/services/mongodb/remote/sync/internal/CoreDocumentSynchronizationConfigUnitTests.kt

-            ),
-            "mongodblocal",
-            ServerEmbeddedMongoClientFactory.getInstance()
+        StitchAppClientInfo(


Looks like this file can be reverted.

edaniels · 2018-10-10T12:36:19Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt

+        val bsonCodec = BsonDocumentCodec()
+
+        // fetch a new namespace, creating a new config
+        val nsConfig: NamespaceSynchronizationConfig = dataSynchronizer.getNamespaceConfig(namespace)


[nit] I think you can get rid of dataSynchronizer.getNamespaceConfig and getNamespaceConfig as being exposed. Instead you should be able to utilize the top level public methods on the data synchronizer instead to act as a normal consumer which would make this a stronger test.

Might it make more sense to do that in an integration test?

I don't think so. The unit test here is aiming to check what happens if you are to configure twice. The first configuration should end up starting the listener. We don't need to reach that far into the implementation details. It ends up making the test brittle and not covering the actual aspect you want to test via the public API.

coveralls · 2018-10-10T13:41:56Z

Coverage decreased (-26.6%) to 26.408% when pulling 17ae642 on jsflax:STITCH-1950 into 5a6a921 on mongodb:master.

coveralls · 2018-10-10T13:41:56Z

Coverage increased (+2.4%) to 55.365% when pulling 6842550 on jsflax:STITCH-1950 into 5a6a921 on mongodb:master.

adamchel

just one nit (that affects a bunch of places), but otherwise looks I good I think once you make Eric's changes

adamchel · 2018-10-10T14:15:44Z

...es/mongodb-remote/src/main/java/com/mongodb/stitch/android/services/mongodb/remote/Sync.java

   */
-  void configure(final ConflictHandler<DocumentT> conflictHandler,
+  void configure(@NonNull final ConflictHandler<DocumentT> conflictHandler,
                 final ChangeEventListener<DocumentT> changeEventListener,


[nit] if we're labeling the conflict handler @NonNull, I guess we should explicitly mark the rest as @Nullable?

adamchel · 2018-10-10T14:15:54Z

...mote/src/main/java/com/mongodb/stitch/android/services/mongodb/remote/internal/SyncImpl.java


  @Override
-  public void configure(final ConflictHandler<DocumentT> conflictHandler,
+  public void configure(@NonNull final ConflictHandler<DocumentT> conflictHandler,


[nit] ditto

adamchel · 2018-10-10T14:16:34Z

...godb-remote/src/main/java/com/mongodb/stitch/core/services/mongodb/remote/sync/CoreSync.java

   * @param errorListener the error listener to invoke when an irrecoverable error occurs
   */
-  void configure(final ConflictHandler<DocumentT> conflictResolver,
+  void configure(@Nonnull final ConflictHandler<DocumentT> conflictHandler,


[nit] ditto

adamchel · 2018-10-10T14:17:01Z

...rc/main/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/CoreSyncImpl.java


  @Override
-  public void configure(final ConflictHandler<DocumentT> conflictResolver,
+  public void configure(@Nonnull final ConflictHandler<DocumentT> conflictHandler,


[nit] ditto

adamchel · 2018-10-10T14:18:04Z

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

-  public <T> void configure(final MongoNamespace namespace,
-                            final ConflictHandler<T> conflictHandler,
+  public <T> void configure(@Nonnull final MongoNamespace namespace,
+                            @Nonnull final ConflictHandler<T> conflictHandler,


[nit] ditto

adamchel · 2018-10-10T14:28:31Z

...ces/mongodb-remote/src/main/java/com/mongodb/stitch/server/services/mongodb/remote/Sync.java

   *                      during data synchronization
   */
-  void configure(final ConflictHandler<DocumentT> conflictResolver,
+  void configure(@Nonnull final ConflictHandler<DocumentT> conflictResolver,


[nit] ditto

adamchel · 2018-10-10T14:28:40Z

...emote/src/main/java/com/mongodb/stitch/server/services/mongodb/remote/internal/SyncImpl.java


  @Override
-  public void configure(final ConflictHandler<DocumentT> conflictResolver,
+  public void configure(@Nonnull final ConflictHandler<DocumentT> conflictResolver,


[nit] ditto

edaniels

One comment and one offline comment. Other than that LGTM

STITCH-1950 Add collection level sync configuration

9e0d525

jsflax requested a review from edaniels October 9, 2018 20:46

jsflax removed the request for review from edaniels October 9, 2018 21:08

jsflax added 2 commits October 10, 2018 12:23

reloadconfig properly changes configuration settings

67934d8

added comments to testConfigure

6ef1770

jsflax requested review from edaniels and adamchel October 10, 2018 12:06

edaniels reviewed Oct 10, 2018

View reviewed changes

edaniels suggested changes Oct 10, 2018

View reviewed changes

jsflax added 2 commits October 10, 2018 14:12

removed superfluous formatting changes

7c5d841

more formatting reversions

17ae642

adamchel approved these changes Oct 10, 2018

View reviewed changes

jsflax added 7 commits October 10, 2018 16:11

made tests more robust

11721d0

nullable changes, datasync changes

2834054

removed stray print, changed formatting

0045d82

removed unused imports

d899866

removed more unused imports

9def655

removed unchecked cast warning

a752586

now releasing lock on all paths

7c1f0ff

edaniels approved these changes Oct 10, 2018

View reviewed changes

jsflax added 2 commits October 10, 2018 17:47

only use syncLock for isConfigured; remove exposure of isConfigured

3320e7f

removed superfluous check in testConfigure

6842550

jsflax merged commit a28ddad into mongodb:master Oct 10, 2018

STITCH-1950 Add collection level sync configuration #67

STITCH-1950 Add collection level sync configuration #67

Uh oh!

Conversation

jsflax commented Oct 9, 2018

Uh oh!

jsflax commented Oct 9, 2018

Uh oh!

jsflax commented Oct 10, 2018

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

coveralls commented Oct 10, 2018

Uh oh!

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

Uh oh!

Uh oh!

adamchel 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!

edaniels left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Oct 10, 2018 •

edited

Loading