Add Client Operation Timeout to Change Streams #1321

vbabanin · 2024-02-28T23:02:56Z

Changes made:

Trigger timeout error when maxAwaitTimeMS exceeds a timeoutMS.
Apply timeoutMS to initial aggregate and subsequent next calls, without appending maxTimeMS to getMore.
Attempt to resume change stream on subsequent next call if previous next failed with timeout error.
Use maxAwaitTimeMS as maxTimeMS in getMore commands.
Preserve change stream validity upon timeout errors.

NOTE

The Reactive Streams specification prevents us from allowing a subsequent next call (event in reactive terms) after a timeout error, conflicting with the CSOT spec requirement not to invalidate the change stream and to try resuming and establishing a new change stream on the server. We immediately let users know about a timeout error, which then closes the stream/publisher.

JAVA-4054

- Trigger timeout error when maxAwaitTimeMS exceeds a timeoutMS. - Apply timeoutMS to initial aggregate and subsequent next calls, without appending maxTimeMS to getMore. - Utilize maxAwaitTimeMS as maxTimeMS in getMore commands. - Preserve change stream validity upon timeout errors. JAVA-4054

JAVA-4054

vbabanin · 2024-03-05T01:13:42Z

driver-core/src/main/com/mongodb/internal/TimeoutContext.java

@@ -165,7 +165,7 @@ public TimeoutSettings getTimeoutSettings() {
    }

    public long getMaxAwaitTimeMS() {
-        return hasTimeoutMS() ? 0 : timeoutSettings.getMaxAwaitTimeMS();


In accordance with the CSOT specifications for the tailable-await cursor:

If the maxAwaitTimeMS option is set, drivers MUST use it as the maxTimeMS field on getMore commands.

The logic to determine whether maxAwaitTimeMS should be utilized for getMore is located in the AggregateOperationImpl and FindOperation.

JAVA-4054

rozza

A couple of questions, its looking good just mainly things to help the maintainability in the future.

Also lets document in the sync drivers that a MongoOperationTimeoutException is not terminal for a change stream cursor and calling next() again may produce results.

rozza · 2024-03-06T16:15:03Z

driver-core/src/main/com/mongodb/internal/operation/CommandBatchCursor.java

@@ -59,6 +59,9 @@
 class CommandBatchCursor<T> implements AggregateResponseBatchCursor<T> {

    private final MongoNamespace namespace;
+    /**
+     * maxAwaitTimeMS
+     */


Nit - no need for this javadoc

rozza · 2024-03-06T16:15:44Z

driver-core/src/main/com/mongodb/internal/operation/CommandBatchCursor.java

@@ -258,6 +261,9 @@ private CommandCursorResult<T> toCommandCursorResult(final ServerAddress serverA
        return commandCursorResult;
    }

+    void setCloseImmediately(final boolean closeImmediately) {


Would be good to explain what his is and why its needed? I dont fully understand the reasoning here.

The current implementation automatically resets the timeoutMS for cursor closure, ensuring that the cursor has sufficient time to release resources. This behavior particularly impacts the ChangeStreamCursor which enforces a unified timeout duration for the entire 'next' operation, including resume attempts and the internal process of closing the cursor.

The method modifies this behavior with respect to timeouts. When enabled, it prevents the automatic reset of the cursor's timeout for closing.

I realized that the method name is not reflecting its purpose. Therefore, I have updated the name to setCloseWithoutTimeoutReset and added the JavaDoc documentation for clarity.

driver-core/src/test/unit/com/mongodb/internal/operation/ChangeStreamBatchCursorTest.java

rozza · 2024-03-06T16:27:59Z

driver-core/src/main/com/mongodb/internal/operation/ChangeStreamBatchCursor.java

@@ -192,14 +217,34 @@ <R> R resumeableOperation(final Function<AggregateResponseBatchCursor<RawBsonDoc
                    throw MongoException.fromThrowableNonNull(t);
                }
            }
-            wrapped.close();
+            resumeChangeStream();


This is weird as resumeChangeStream is also called via resumeAfterTimeout - can this be simplified?

I've simplified the code to avoid using resumeAfterTimeout. If you have specific suggestions, please let me know.

rozza · 2024-03-06T16:29:43Z

driver-core/src/main/com/mongodb/internal/operation/ChangeStreamBatchCursor.java

+     * Indicates whether the last {@code next} call resulted in a {@link MongoOperationTimeoutException}. If {@code true},
+     * indicates a timeout occurred, prompting an attempt to resume the change stream on the next call.
+     */
+    private boolean lastOpTimeoutStatus;


The naming / use of this flag is confusing. Can you clarify in the code / docs that throwing a MongoOperationTimeoutException doesn't invalidate the cursor.

The reason for this is in 6 months we'll all have forgotten about this special and strange CSOT behavior.

Acknowledged. I have renamed the field and added additional class level javaDoc. Please let me know if there is anything to add.

Remove additional call to resumeChangeStream. JAVA-4054

JAVA-4054

rozza

Looking really good.

Need to add to the public ChangeStreamIterable Javadoc so its user discoverable.

 * <p>
 * A {@link MongoOperationTimeoutException} does not invalidate the {@link ChangeStreamBatchCursor}, but is immediately propagated to the caller.
 * Subsequent method call will attempt to resume operation by establishing a new change stream on the server, without doing {@code getMore}
 * request first.
 * </p>

JAVA-5322

rozza

LGTM!

rozza · 2024-03-13T12:24:19Z

I noticed that the test failures are unrelated but we should keep an eye on them as there maybe more racy tests to review / fix in the future.

vbabanin and others added 13 commits February 28, 2024 15:01

Format code

d8760ab

JAVA-4054

Fix test.

961481e

JAVA-4054

Adjust timeouts in tests.

37a9994

JAVA-4054

Adjust timeouts in tests.

728d611

JAVA-4054

Add CSOT to asynchronous change stream cursor.

de200fe

JAVA-4054

Fix tests.

da6eaea

JAVA-4054

Account for RTT.

4c44db9

JAVA-4054

Add assumption to the test.

4fd4d96

JAVA-4054

Reset onEachOperator hook after tests.

de6a9b3

JAVA-4054

Improve test error output.

cb69eb4

JAVA-4054

Improve test error output.

9507840

JAVA-4054

Change test data order.

0a0e606

JAVA-4054

vbabanin commented Mar 5, 2024

View reviewed changes

vbabanin added 2 commits March 4, 2024 23:00

Remove BatchCursorInterceptor.

161667e

JAVA-4054

Fix checkstyle.

ecac544

JAVA-4054

vbabanin marked this pull request as ready for review March 5, 2024 07:56

vbabanin requested a review from rozza March 5, 2024 07:56

rozza requested changes Mar 6, 2024

View reviewed changes

vbabanin added 2 commits March 6, 2024 16:50

Add additional javadoc.

6798ceb

Remove additional call to resumeChangeStream. JAVA-4054

Merge branch 'CSOT' into JAVA-4054

5b7eeb7

vbabanin requested a review from rozza March 7, 2024 00:56

Fix typo.

e8f3cf0

JAVA-4054

rozza requested changes Mar 8, 2024

View reviewed changes

Add javadoc.

84b0971

JAVA-5322

vbabanin requested a review from rozza March 12, 2024 06:17

rozza approved these changes Mar 13, 2024

View reviewed changes

vbabanin merged commit ab1ec84 into mongodb:CSOT Mar 14, 2024
50 of 58 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Client Operation Timeout to Change Streams #1321

Add Client Operation Timeout to Change Streams #1321

vbabanin commented Feb 28, 2024 •

edited

Loading

vbabanin Mar 5, 2024 •

edited

Loading

rozza left a comment

rozza Mar 6, 2024

rozza Mar 6, 2024

vbabanin Mar 6, 2024 •

edited

Loading

rozza Mar 6, 2024

vbabanin Mar 7, 2024

rozza Mar 6, 2024

vbabanin Mar 7, 2024

rozza left a comment

rozza left a comment

rozza commented Mar 13, 2024

Add Client Operation Timeout to Change Streams #1321

Add Client Operation Timeout to Change Streams #1321

Conversation

vbabanin commented Feb 28, 2024 • edited Loading

Changes made:

NOTE

vbabanin Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

rozza left a comment

Choose a reason for hiding this comment

rozza Mar 6, 2024

Choose a reason for hiding this comment

rozza Mar 6, 2024

Choose a reason for hiding this comment

vbabanin Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

rozza Mar 6, 2024

Choose a reason for hiding this comment

vbabanin Mar 7, 2024

Choose a reason for hiding this comment

rozza Mar 6, 2024

Choose a reason for hiding this comment

vbabanin Mar 7, 2024

Choose a reason for hiding this comment

rozza left a comment

Choose a reason for hiding this comment

rozza left a comment

Choose a reason for hiding this comment

rozza commented Mar 13, 2024

vbabanin commented Feb 28, 2024 •

edited

Loading

vbabanin Mar 5, 2024 •

edited

Loading

vbabanin Mar 6, 2024 •

edited

Loading