Implementing watch for Java, Mongo 3.6, Fix #146 #145

doctorpangloss · 2018-04-11T05:43:43Z

Ready to merge

Also fixes #146, fixes #144

The basics are all in there. This should be ready for review.
Other notes:

Mongo 3.6 through the flapdoodle embed mongo package doesn't work on Windows 10. This is the underlying issue: https://jira.mongodb.org/browse/SERVER-26028
Using pipelines is pretty wonky too. I don't think many vertx API methods accept JsonArrays.
The actual change stream implementation is full of surprises. What is the best way to document this? People will constantly be filing bugs with you over stuff that is just changeStreams wonkiness. I've expanded the watch jsdoc to try to address common issues.
It is exceptionally surprising that a watch's handler is evaluated after a match insert's callback, even if the watch was registered before the insert. Any interest in addressing this?
$project, $replaceRoot, and $redact pipeline operators will glitch out in all versions of the mongo java driver due to https://jira.mongodb.org/browse/JAVA-2828 . The code that supports those operations (watchReplaceRoot) is written and will throw new UnsupportedOperationException until that patch is done. What do you think about this? Should MongoClientImpl inspect the user's pipeline for usages of those (currently) unsupported operators?

karianna

A view comments without really knowing the codebase that well - so see it as nit picking in isolation. I'll leave it to @johnoliver to give more meaningful semantic feedback

karianna · 2018-04-11T15:03:51Z

vertx-mongo-client/src/main/java/io/vertx/ext/mongo/MongoClientChange.java

+  public static final String OP_DELETE = "delete";
+  public static final String OP_INVALIDATE = "invalidate";
+
+  private List<String> removedFields;


Should this be a List<JsonObject>?

karianna · 2018-04-11T15:04:10Z

vertx-mongo-client/src/main/java/io/vertx/ext/mongo/MongoClientChange.java

+  private JsonObject updatedFields;
+  private JsonObject fullDocument;
+  private JsonObject resumeToken;
+  private String operationType;


Could be an enum?

karianna · 2018-04-11T15:04:44Z

vertx-mongo-client/src/main/java/io/vertx/ext/mongo/MongoClientChange.java

+  public MongoClientChange(JsonObject json) {
+    this();
+    final JsonArray removedFields = json.getJsonArray("removedFields");
+    if (removedFields != null) {


Could stream / filter this as opposed to using traditional if / for?

karianna · 2018-04-11T15:06:21Z

vertx-mongo-client/src/main/java/io/vertx/ext/mongo/WatchOptions.java

+  }
+
+  public WatchOptions(JsonObject options) {
+    resumeAfter = options.getJsonObject("resumeAfter");


Some magic strings could be extract here

I am following the convention from other DataObject classes in here, should I make this change?

In that case no.

karianna · 2018-04-11T15:07:20Z

vertx-mongo-client/src/main/java/io/vertx/ext/mongo/impl/MongoClientImpl.java

-import io.vertx.ext.mongo.BulkWriteOptions;
-import io.vertx.ext.mongo.FindOptions;
-import io.vertx.ext.mongo.IndexOptions;
+import io.vertx.ext.mongo.*;


Prefer explicit imports

karianna · 2018-04-11T15:08:22Z

vertx-mongo-client/src/main/java/io/vertx/ext/mongo/impl/MongoClientImpl.java

+
+    try {
+      List<Bson> pipelineBson = new ArrayList<>();
+      for (int i = 0; i < pipeline.size(); i++) {


can we use an enhanced for loop here?

This should actually probably use the JsonObjectBsonAdapter

karianna · 2018-04-11T15:09:08Z

vertx-mongo-client/src/main/java/io/vertx/ext/mongo/impl/MongoClientImpl.java

+      MongoIterableStream<ChangeStreamDocument<JsonObject>> adaptor = new MongoIterableStream<>(view, context, options.getBatchSize());
+      resultHandler.handle(Future.succeededFuture(new MongoClientWatchIterableImpl(adaptor)));
+
+    } catch (Exception unhandledEx) {


Are there not CheckedExceptions that can be caught? Or are you worried about the random RuntimeExcpetion like an NPE?

karianna · 2018-04-11T15:10:01Z

vertx-mongo-client/src/main/kotlin/io/vertx/kotlin/ext/mongo/MongoClientChange.kt

@@ -0,0 +1,9 @@
+package io.vertx.kotlin.ext.mongo


I assume this is generated code? Same goes for the other Kotlin pieces

karianna · 2018-04-11T15:11:08Z

vertx-mongo-client/src/test/java/io/vertx/ext/mongo/WatchTest.java

+  }
+
+  @Test
+  public void testInsertChangeStream() {


Maybe slight name change. We're asserting that our watch operation 'sees' the inset, correct?

doctorpangloss · 2018-04-11T17:27:34Z

(Resolved)

karianna · 2018-04-11T18:18:36Z

I'll hand that question to @johnoliver :-)

doctorpangloss · 2018-04-11T22:00:33Z

All the tests now run against a replica set, so tests will be a little slower.

Otherwise, my changes seem to pass tests. We probably need a specific test for what that executeBlocking is intended for (the batch response happens inside a java.nio call stack, naturally, so it's not like there's a "Mongo Thread" or anything blocking like that).

Fixes $setOnInsert no longer silently superseding $set on 3.6. Fixes findBatch creating too many connections. Note: Tests against mongo 3.6 cannot run on Windows with current flapdoodle embed mongo.

johnoliver · 2018-04-18T17:16:57Z

vertx-mongo-client/src/main/java/io/vertx/ext/mongo/MongoClientChange.java

+
+@DataObject
+public class MongoClientChange {
+  private static final ThreadLocal<JsonObjectCodec> codec = ThreadLocal.withInitial(() -> new JsonObjectCodec(new JsonObject()));


Is there a reason for having a codec per thread?

Nope, wasn't sure if the codec object was thread safe

johnoliver · 2018-04-18T17:32:56Z

vertx-mongo-client/src/main/java/io/vertx/ext/mongo/impl/MongoClientImpl.java

+      // If the ID is inside $set, remove it
+      // The error that this would cause would start to get enforced in 3.6 (i.e., $setOnInsert no longer silently
+      // supersedes $set).
+      if (update.containsKey("$set") && update.getJsonObject("$set").containsKey("_id")) {


not 100% on this one, I do accept that if someone changes the _id to a different value, then it is almost certainly an error by the user, and the mongo will throw an error. However by removing the _id we may be covering up the downstream mistake, and would get into a situation where it looks like we have successfully saved a record from the users point of view but not with the _id that they asked for.

I am a bit 50/50 on what is better here, encode the _id and allow the user to discover their mistake, or remove it.

also this fixes #144

The tests would have to change because three of them fail due to this issue right now. I agree with you, but I'm less certain about the object ID patching code and how to change it.

Looking at this again, I think it's actually better to keep this exact line in, because it passes the old test and maintains 100% compatibility with the current users (the issues reported) right now.

johnoliver · 2018-04-18T17:38:47Z

vertx-mongo-client/src/main/java/io/vertx/ext/mongo/impl/MongoClientImpl.java

+    } else {
+      if (options.getFullDocument() != null
+        && !options.getFullDocument().equals(WatchOptions.FULL_DOCUMENT_UPDATE_LOOKUP)) {
+        resultHandler.handle(Future.failedFuture(new IllegalArgumentException("fullDocument must be null or \"updatedLookup\"")));


Think the error message makes a bit more sense as:

fullDocument option must be either null or "updatedLookup"

Will change

johnoliver · 2018-04-18T18:46:11Z

vertx-mongo-client/src/main/java/io/vertx/ext/mongo/impl/MongoClientImpl.java

+      Context context = vertx.getOrCreateContext();
+      if (replacedRootDocument) {
+        MongoIterableStream<JsonObject> adaptor = new MongoIterableStream<>(view.withDocumentClass(JsonObject.class), context, options.getBatchSize());
+        resultHandler.handle(Future.succeededFuture((MongoClientChangeStream<T>)new MongoClientReplacedRootStreamImpl(adaptor)));


I think this casting may show a mistake in how control is handled here, createWatchIterable could return ChangeStreamIterable<JsonObject> then let the upstream methods deal with converting it to either a MongoClientChangeStream<JsonObject> or MongoClientChangeStream<MongoClientChange>

I'm going to remove the watchReplaceRoot method so this won't be necessary. Based on my conversation with the mongo folks, they still need to think about how to allow $redact, $replaceRoot etc. in the watch pipeline. They're probably going to allow the caller to disable the driver's saving of resume tokens.

johnoliver · 2018-04-18T19:07:14Z

vertx-mongo-client/src/test/java/io/vertx/ext/mongo/MongoTestBase.java

        build();
      exe = MongodStarter.getDefaultInstance().prepare(config);
      exe.start();
+      final JsonObject replSetInitiateConfig = new JsonObject();


Is there a reason to configure the replSet?

Yes, watch requires a replica set to work.

Or, if you mean there's a way to do that without configuring the replica set, I'd do that instead. But as far as I know, I have to issue this command to get everything going correctly.

johnoliver · 2018-04-18T19:16:28Z

vertx-mongo-service/src/main/java/io/vertx/ext/mongo/MongoService.java

+  @Override
+  @Fluent
+  @GenIgnore
+  default MongoClient watch(String collection, JsonArray pipeline, WatchOptions options, Handler<AsyncResult<MongoClientChangeStream<MongoClientChange>>> resultHandler) {


I know they are unsupported but should still return MongoService

Will change.

johnoliver · 2018-04-18T19:26:47Z

regarding the executeBlocking it is probably not needed it my have been because the Mongo docs for AsyncBatchCursor.next says:

Returns the next batch of results.  A tailable cursor will block until another batch exists.

Which may give the impression that it is a blocking method call, but you would hope it is not.

doctorpangloss · 2018-04-18T22:35:41Z

Based on inspecting the mongo code, the cursor uses "An asynchronous connection to a MongoDB server with non-blocking operations." So that's probably not what they meant.

doctorpangloss · 2018-04-18T22:50:54Z

Looking more carefully, mongo supports getting a Netty event loop group instead of using nio2. You may consider always doing this for the end user if context is instanceof EventLoopContext.

Apparently in the postgresql driver they do this:

https://github.com/vert-x3/vertx-mysql-postgresql-client/blob/master/src/main/java/io/vertx/ext/asyncsql/impl/VertxEventLoopExecutionContext.java

doctorpangloss · 2018-04-18T22:52:37Z

Also this passes tests with the 3.6.0-SNAPSHOT along with 3.5.1

vietj · 2018-04-26T12:06:36Z

can we merge this @karianna I had to set mongo version to 3.4 to get the project building, if we merge this it could be set back to 3.6

vietj · 2018-04-26T12:06:56Z

I would like also to backport to 3.5.2

karianna · 2018-04-26T14:51:24Z

@doctorpangloss Hmm, this is reporting lots of conflicts now, not sure how that came to be. If you can resolve those then @johnoliver and I can take a final look

doctorpangloss · 2018-05-10T06:21:58Z

I think I'll revisit this again later. I am happy with my fork of good, working code.

vietj added the to review label Apr 11, 2018

karianna added this to the 3.x.x milestone Apr 11, 2018

karianna requested review from karianna and johnoliver April 11, 2018 15:02

karianna reviewed Apr 11, 2018

View reviewed changes

karianna added the enhancement label Apr 11, 2018

doctorpangloss force-pushed the 3.5.1-watch branch from a2169a5 to 3684b00 Compare April 11, 2018 21:37

doctorpangloss changed the title ~~[WIP] Implementing watch for Java, Mongo 3.6~~ Implementing watch for Java, Mongo 3.6 Apr 11, 2018

doctorpangloss changed the title ~~Implementing watch for Java, Mongo 3.6~~ [Blocked by dependencies] Implementing watch for Java, Mongo 3.6 Apr 11, 2018

doctorpangloss force-pushed the 3.5.1-watch branch from 6ee43af to 8106469 Compare April 16, 2018 00:32

doctorpangloss changed the title ~~[Blocked by dependencies] Implementing watch for Java, Mongo 3.6~~ Implementing watch for Java, Mongo 3.6 Apr 16, 2018

doctorpangloss mentioned this pull request Apr 16, 2018

id field issue in 3.6 (fixed in PR #145) #146

Closed

doctorpangloss added 2 commits April 15, 2018 17:50

Implementing watch for Java, Mongo 3.6

8412964

Fixes $setOnInsert no longer silently superseding $set on 3.6. Fixes findBatch creating too many connections. Note: Tests against mongo 3.6 cannot run on Windows with current flapdoodle embed mongo.

Use new dependency

45e2fe8

doctorpangloss force-pushed the 3.5.1-watch branch from 543491b to 45e2fe8 Compare April 16, 2018 00:52

doctorpangloss changed the title ~~Implementing watch for Java, Mongo 3.6~~ Implementing watch for Java, Mongo 3.6, Fix #146 Apr 16, 2018

johnoliver reviewed Apr 18, 2018

View reviewed changes

Notes from review

76d79b0

vietj modified the milestones: 3.x.x, 3.6.0 Apr 26, 2018

doctorpangloss closed this May 10, 2018

vietj removed the to review label May 10, 2018

karianna added the wontfix label May 10, 2018

karianna modified the milestones: 3.6.0, 3.x.x May 31, 2018

karianna modified the milestones: 3.x.x, 3.6.0 Dec 4, 2018

Implementing watch for Java, Mongo 3.6, Fix #146 #145

Implementing watch for Java, Mongo 3.6, Fix #146 #145

Conversation

doctorpangloss commented Apr 11, 2018 • edited Loading

karianna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doctorpangloss commented Apr 11, 2018 • edited Loading

karianna commented Apr 11, 2018

doctorpangloss commented Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doctorpangloss Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnoliver commented Apr 18, 2018

doctorpangloss commented Apr 18, 2018

doctorpangloss commented Apr 18, 2018

doctorpangloss commented Apr 18, 2018

vietj commented Apr 26, 2018

vietj commented Apr 26, 2018

karianna commented Apr 26, 2018

doctorpangloss commented May 10, 2018

doctorpangloss commented Apr 11, 2018 •

edited

Loading

doctorpangloss commented Apr 11, 2018 •

edited

Loading

doctorpangloss commented Apr 11, 2018 •

edited

Loading

doctorpangloss Apr 18, 2018 •

edited

Loading