Allow configuring the monitoring protocol to use; use the polling protocol in a FaaS environment by default #1313

stIncMale · 2024-02-21T21:00:47Z

Reviewers:

@vbabanin picked up automatically via @mongodb/dbx-java.
@jyemin picked up manually as @mongodb/dbx-java does not work twice.

The initial 6 commits are organized in such a way that a reviewer may review them one by one. They don't step on each other's toes, and represent separate work items.

JAVA-4936

This change was supposed to be done as part of mongodb@c8b028a JAVA-4936

Note that despite this commit marking 1.16 as supported, it actually is not. The existing practice is to mark as supported all the versions up to the one that is either fully or partially supported, despite them being likely not actually supported. When someone discovers that a feature is used in a test, but is not implemented, then he implements that feature. JAVA-4936

This change is with accordance to source/faas-automated-testing/faas-automated-testing.rst JAVA-4936

This change is in accordance with source/uri-options/uri-options.rst. JAVA-4936

This change is in accordance with source/server-discovery-and-monitoring/server-monitoring.rst. JAVA-4936

stIncMale · 2024-02-22T20:31:14Z

driver-core/src/main/com/mongodb/internal/connection/DefaultServerMonitor.java

-    private final RoundTripTimeRunnable roundTripTimeMonitor;
+    private final ServerMonitor monitor;
+    /**
+     * Must be guarded by {@link #lock}.
+     */
+    @Nullable
+    private RoundTripTimeMonitor roundTripTimeMonitor;
    private final ExponentiallyWeightedMovingAverage averageRoundTripTime = new ExponentiallyWeightedMovingAverage(0.2);
-    private final Thread roundTripTimeMonitorThread;


The changes required both roundTripTimeMonitor & roundTripTimeMonitorThread to become null-able. I simplified that by replacing these two fields with a single field. However, this change lead to an inconsistent code having a single field roundTripTimeMonitor and two coupled fields monitor & monitorThread. To avoid such inconsistency, I also replaced the latter with a single field.

Thank you, the code looks cleaner now 👍

stIncMale · 2024-02-22T22:50:29Z

.../src/test/functional/com/mongodb/client/unified/UnifiedServerDiscoveryAndMonitoringTest.java

+    public static void skipTests(final BsonDocument definition) {
+        String description = definition.getString("description", new BsonString("")).getValue();
+        assumeFalse("Skipping because our server monitoring events behave differently for now",
+                description.equals("connect with serverMonitoringMode=auto >=4.4"));
+        assumeFalse("Skipping because our server monitoring events behave differently for now",
+                description.equals("connect with serverMonitoringMode=stream >=4.4"));
+    }


This will be undone by JAVA-5230 as per https://jira.mongodb.org/browse/JAVA-5230?focusedCommentId=6118489&focusedId=6118489&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-6118489.

stIncMale · 2024-02-22T22:51:08Z

.../unified-test-format/server-discovery-and-monitoring/serverMonitoringMode-java-specific.json

This will be undone by JAVA-5230 as per https://jira.mongodb.org/browse/JAVA-5230?focusedCommentId=6118489&focusedId=6118489&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-6118489.

driver-core/src/main/com/mongodb/connection/ServerMonitoringMode.java

vbabanin · 2024-02-28T07:12:17Z

driver-core/src/main/com/mongodb/internal/connection/DefaultServerMonitor.java

@@ -165,8 +192,7 @@ public void run() {
                    logStateChange(previousServerDescription, currentServerDescription);
                    sdamProvider.get().update(currentServerDescription);

-                    if (((connection == null || shouldStreamResponses(currentServerDescription))


is this check "connection == null" not needed anymore?

I thought I had proven to myself that the check was redundant by transforming to

boolean shouldStream = currentServerDescription.getTopologyVersion() != null; if (((connection == null || shouldStream) && shouldStream && currentServerDescription.getType() != UNKNOWN) || (connection != null && connection.hasMoreToCome()) || (currentServerDescription.getException() instanceof MongoSocketException && previousServerDescription.getType() != UNKNOWN)) { continue; }

and then allowing Intellij to simplify the expression, but when I tried it again IntelliJ didn't offer the simplification.

This is a truly horrible conditional, and has been the source of at least one bug that I remember.

is this check "connection == null" not needed anymore?

It had no effect, so I removed it. The following demonstrates why the connection == null expression had no effect.

Here is the original conditional, but reformatted to simplify perception:

( (connection == null || shouldStreamResponses(currentServerDescription)) && currentServerDescription.getTopologyVersion() != null && currentServerDescription.getType() != UNKNOWN ) || (connection != null && connection.hasMoreToCome()) || ( currentServerDescription.getException() instanceof MongoSocketException && previousServerDescription.getType() != UNKNOWN )

It consists of three Boolean expressions ORed (||) together. Only the first one of those is modified in the PR.

Let's alias the connection == null expression as x.

Note that the expressions shouldStreamResponses(currentServerDescription) and currentServerDescription.getTopologyVersion() != null are equivalent (well, were equivalent at the time of the commit that simplified the expression). Let's alias them as s.

Now the first ORed Boolean expression can be written as

( (x || s) && s && currentServerDescription.getType() != UNKNOWN )

If s is false, this expression evaluates to false. If s is true, this expression evaluates to the same value as currentServerDescription.getType() != UNKNOWN. As you can see, x affects the outcome is neither case, and the expression can be simplified to

( s && currentServerDescription.getType() != UNKNOWN )

Thanks for the clarification, @stIncMale. Now i see, It is indeed redundant check.

i am curious about the original purpose of this check, given it wasn't functional. My assumption was that it might have aimed to bypass waitForNext() on lookup failure as connection is nullified in lookup method. However, the presence of (currentServerDescription.getException() instanceof MongoSocketException && previousServerDescription.getType() != UNKNOWN)seems to cover scenario aligning with the spec, which mandates immediate retry on network errors when the server was previously in a known state.

#. If this was a network error and the server was in a known state before the error, the client MUST NOT sleep and MUST begin the next check immediately.

Feel free to resolve the conversation if @jyemin has no further comments.

driver-core/src/main/com/mongodb/internal/connection/DefaultServerMonitor.java

driver-core/src/main/com/mongodb/event/ServerHeartbeatStartedEvent.java

driver-core/src/main/com/mongodb/connection/ServerMonitoringMode.java

driver-core/src/main/com/mongodb/ConnectionString.java

driver-core/src/main/com/mongodb/internal/connection/DefaultClusterableServerFactory.java

driver-core/src/main/com/mongodb/internal/connection/DefaultServerMonitor.java

jyemin · 2024-02-28T14:21:57Z

driver-core/src/main/com/mongodb/internal/connection/DefaultServerMonitor.java

@@ -165,8 +192,7 @@ public void run() {
                    logStateChange(previousServerDescription, currentServerDescription);
                    sdamProvider.get().update(currentServerDescription);

-                    if (((connection == null || shouldStreamResponses(currentServerDescription))


I thought I had proven to myself that the check was redundant by transforming to

boolean shouldStream = currentServerDescription.getTopologyVersion() != null; if (((connection == null || shouldStream) && shouldStream && currentServerDescription.getType() != UNKNOWN) || (connection != null && connection.hasMoreToCome()) || (currentServerDescription.getException() instanceof MongoSocketException && previousServerDescription.getType() != UNKNOWN)) { continue; }

and then allowing Intellij to simplify the expression, but when I tried it again IntelliJ didn't offer the simplification.

This is a truly horrible conditional, and has been the source of at least one bug that I remember.

…de.java Co-authored-by: Viacheslav Babanin <frest0512@gmail.com>

JAVA-4936

driver-core/src/main/com/mongodb/connection/ServerMonitoringMode.java

driver-core/src/main/com/mongodb/internal/connection/ServerMonitoringModeUtil.java

…de.java Co-authored-by: Jeff Yemin <jeff.yemin@mongodb.com>

…itoringModeUtil.java Co-authored-by: Jeff Yemin <jeff.yemin@mongodb.com>

vbabanin

LGTM!

jyemin

LGTM!

stIncMale self-assigned this Feb 21, 2024

stIncMale force-pushed the JAVA-4936 branch from c54fcb7 to 78f0a48 Compare February 21, 2024 21:23

stIncMale added 3 commits February 21, 2024 14:41

Support ServerHeartbeatStartedEvent.awaited

33631e1

This change was supposed to be done as part of mongodb@c8b028a JAVA-4936

Update the FaaS prose test

63562c6

This change is with accordance to source/faas-automated-testing/faas-automated-testing.rst JAVA-4936

stIncMale force-pushed the JAVA-4936 branch 2 times, most recently from 931f6e0 to cdd3af9 Compare February 22, 2024 17:11

stIncMale added 3 commits February 22, 2024 13:18

Support the serverMonitoringMode connection string option

1e74b01

This change is in accordance with source/uri-options/uri-options.rst. JAVA-4936

Implement the serverMonitoringMode logic

640fd79

This change is in accordance with source/server-discovery-and-monitoring/server-monitoring.rst. JAVA-4936

Start roundTripTimeMonitor only if the streaming protocol is used

b7e461f

This change is in accordance with source/server-discovery-and-monitoring/server-monitoring.rst. JAVA-4936

stIncMale force-pushed the JAVA-4936 branch from cdd3af9 to b7e461f Compare February 22, 2024 20:19

stIncMale commented Feb 22, 2024

View reviewed changes

stIncMale marked this pull request as ready for review February 22, 2024 22:42

stIncMale requested review from a team, vbabanin and jyemin and removed request for a team February 22, 2024 22:42

stIncMale commented Feb 22, 2024

View reviewed changes

vbabanin reviewed Feb 28, 2024

View reviewed changes

jyemin requested changes Feb 28, 2024

View reviewed changes

stIncMale and others added 6 commits February 28, 2024 10:46

Merge branch 'master' into JAVA-4936

91a6ebb

Update driver-core/src/main/com/mongodb/connection/ServerMonitoringMo…

2f3cdcb

…de.java Co-authored-by: Viacheslav Babanin <frest0512@gmail.com>

faas -> isFunctionAsAServiceEnvironment

bc589fb

JAVA-4936

Fix the race condition in DefaultServerMonitor

315d66c

JAVA-4936

Add more docs for the streaming protocol

00c05ff

JAVA-4936

Remove ServerMonitoringMode.getValue/fromString from the API

fa83aba

JAVA-4936

stIncMale requested a review from vbabanin February 28, 2024 22:41

stIncMale requested a review from jyemin February 28, 2024 22:42

Merge branch 'master' into JAVA-4936

ac346ab

jyemin requested changes Feb 28, 2024

View reviewed changes

driver-core/src/main/com/mongodb/connection/ServerMonitoringMode.java Outdated Show resolved Hide resolved

driver-core/src/main/com/mongodb/internal/connection/ServerMonitoringModeUtil.java Show resolved Hide resolved

stIncMale and others added 2 commits February 28, 2024 17:49

Update driver-core/src/main/com/mongodb/connection/ServerMonitoringMo…

b372ca9

…de.java Co-authored-by: Jeff Yemin <jeff.yemin@mongodb.com>

Update driver-core/src/main/com/mongodb/internal/connection/ServerMon…

128741d

…itoringModeUtil.java Co-authored-by: Jeff Yemin <jeff.yemin@mongodb.com>

stIncMale requested a review from jyemin February 29, 2024 01:18

vbabanin approved these changes Feb 29, 2024

View reviewed changes

stIncMale mentioned this pull request Feb 29, 2024

Add Human OIDC Workflow #1316

Merged

jyemin approved these changes Feb 29, 2024

View reviewed changes

stIncMale merged commit 7295322 into mongodb:master Feb 29, 2024
56 checks passed

stIncMale deleted the JAVA-4936 branch February 29, 2024 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuring the monitoring protocol to use; use the polling protocol in a FaaS environment by default #1313

Allow configuring the monitoring protocol to use; use the polling protocol in a FaaS environment by default #1313

stIncMale commented Feb 21, 2024 •

edited

Loading

stIncMale Feb 22, 2024

vbabanin Feb 29, 2024

stIncMale Feb 22, 2024

stIncMale Feb 22, 2024

vbabanin Feb 28, 2024

jyemin Feb 28, 2024

stIncMale Feb 28, 2024 •

edited

Loading

vbabanin Feb 29, 2024 •

edited

Loading

jyemin Feb 28, 2024

vbabanin left a comment

jyemin left a comment

Allow configuring the monitoring protocol to use; use the polling protocol in a FaaS environment by default #1313

Allow configuring the monitoring protocol to use; use the polling protocol in a FaaS environment by default #1313

Conversation

stIncMale commented Feb 21, 2024 • edited Loading

stIncMale Feb 22, 2024

Choose a reason for hiding this comment

vbabanin Feb 29, 2024

Choose a reason for hiding this comment

stIncMale Feb 22, 2024

Choose a reason for hiding this comment

stIncMale Feb 22, 2024

Choose a reason for hiding this comment

vbabanin Feb 28, 2024

Choose a reason for hiding this comment

jyemin Feb 28, 2024

Choose a reason for hiding this comment

stIncMale Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

vbabanin Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

jyemin Feb 28, 2024

Choose a reason for hiding this comment

vbabanin left a comment

Choose a reason for hiding this comment

jyemin left a comment

Choose a reason for hiding this comment

stIncMale commented Feb 21, 2024 •

edited

Loading

stIncMale Feb 28, 2024 •

edited

Loading

vbabanin Feb 29, 2024 •

edited

Loading