-
Notifications
You must be signed in to change notification settings - Fork 1.5k
CSOT: Network failure handling and session management #1396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Mark session as dirty when MongoSocketException occurs. - Skip killCursors command execution when getMore fails in LOAD_BALANCER mode. JAVA-5474 JAVA-5476
JAVA-5474 JAVA-5476
JAVA-5474 JAVA-5476
# Conflicts: # driver-core/src/main/com/mongodb/internal/connection/InternalStreamConnection.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question - but looking good.
} | ||
|
||
protected boolean isOperationTimeoutFromSocketException(final Throwable e) { | ||
return e instanceof MongoOperationTimeoutException && e.getCause() instanceof MongoSocketException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can e.getCause()
return null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it can return null in cases where MongoOperationTimeoutException
was not caused by any blocking operation . However, instanceOf
will return false
when it is null.
/** | ||
* <p>This class is not part of the public API and may be removed or changed at any time</p> | ||
*/ | ||
public abstract class AbstractProtocolExecutor implements ProtocolExecutor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this abstract class need to exist? Can these helper methods just be added to ProtocolExecutor
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, the methods could be moved to the interface since they are stateless, but this would make them public. I've opted to keep them protected
within the abstract class to maintain better encapsulation and clearer boundaries in the codebase. If you feel that we should move them to the interface, I am open to doing that. Please let me know your thoughts.
I've also relocated the isMongoSocketException
and isOperationTimeoutFromSocketException
to ExceptionUtils
for broader reuse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all internal
code, it just adds an extra layer for helper style methods. Looks like this abstract class could be removed and shouldMarkSessionDirty
be moved to a helper.
However, if you are keen to keep it protected then its ok with me.
any(), any(), any(), any(), any(), any()); | ||
verify(mockConnection, times(1)).commandAsync(eq(NAMESPACE.getDatabaseName()), | ||
argThat(bsonDocument -> bsonDocument.containsKey("getMore")), any(), any(), any(), any(), any()); | ||
verify(mockConnection, never()).commandAsync(eq(NAMESPACE.getDatabaseName()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a checkstyle issue to be fixed in the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
This PR introduces enhancements to how network failures are handled when Client-Side Operation Timeout (CSOT) is enabled to ensure compliance with the specifications for serverless topologies operating in LOAD_BALANCED mode and driver sessions.
Changes:
After a getMore network failure the pinned connection, the killCursors command is not executed adhering to the specification.
When a socket timeout occurs with CSOT enabled, the associated server session is marked as dirty, adhering to the driver sessions. specification.
References:
JAVA-5474
JAVA-5476