Obtain waitQueue and connection timeouts from OperationContext #1228
Conversation
| } | ||
|
|
||
| public int getConnectTimeoutMs() { | ||
| return (int) getTimeoutSettings().getConnectTimeoutMS(); |
There was a problem hiding this comment.
This returns an int. I think it would be more consistent to return a Timeout, but it was used in a way where 0 indicated infinite timeout (rather than the usual negative is infinite, see above). I could not find what the user should use to indicate infinite connect timeout specified in our docs.
There was a problem hiding this comment.
This is because with Java sockets a timeout cannot be negative and 0 means infinite. See: socket.connect
There was a problem hiding this comment.
Right, though I think that we should probably say the same in our own docs. (I didn't confirm that all paths used an API where 0 meant infinite, but I think this must be the case.)
| this(timeoutMS, null, serverSelectionTimeoutMS, connectTimeoutMS, readTimeoutMS, 0, 0, 0, null); | ||
| final long serverSelectionTimeoutMS, final long connectTimeoutMS, final long readTimeoutMS, | ||
| @Nullable final Long timeoutMS, final long maxWaitTimeMS) { | ||
| this(timeoutMS, null, serverSelectionTimeoutMS, connectTimeoutMS, readTimeoutMS, 0, 0, 0, null, maxWaitTimeMS); |
There was a problem hiding this comment.
I added maxWaitTimeMS, in a way that seemed consistent with readTimeoutMS, but I was also not sure whether we intended to pass these in, since both were deprecated. Should these really be passed in, or should they be set using one of the "with" methods below?
There was a problem hiding this comment.
Its needed in the constructor, so it can be copied via the with methods.
We should eventually make this constructor private or annotate visibility test only.
| OPERATION_CONTEXT.getSessionContext(), | ||
| new TimeoutContext(new TimeoutSettings(30_000, 10_000, 0, null, 0)), | ||
| OPERATION_CONTEXT.getServerApi()); | ||
| connections.add(provider.get(operationContextWithZeroMaxWait)); |
There was a problem hiding this comment.
Note that this constructs a new operation context, as before with a 0 wait time (which I believe is intentional in this test), rather than just using OPERATION_CONTEXT.
| @@ -41,6 +44,24 @@ public OperationContext(final RequestContext requestContext, final SessionContex | |||
| this(NEXT_ID.incrementAndGet(), requestContext, sessionContext, timeoutContext, serverApi); | |||
| } | |||
|
|
|||
|
|
|||
| public static OperationContext todoOperationContext() { | |||
| // TODO (CSOT) should be removed; used at locations that require an OC, but which do not yet have one available | |||
There was a problem hiding this comment.
These TODOs are intentional, and would get merged into the CSOT branch (which we would check for TODOs before merging, as per a comment there).
I left this open, because I think this would be more appropriate to supply during the crypt work.
| public static OperationContext nonUserOperationContext(final MongoClientSettings settings) { | ||
| // TODO (CSOT) below is placeholder, validate correctness (serverApi/timeoutSettings might | ||
| // TODO (CSOT) need to be passed in instead) | ||
| TimeoutSettings timeoutSettings = TimeoutSettings.create(settings); |
There was a problem hiding this comment.
This PR does not implement provision of non-user operation contexts (for example, monitoring, etc.), and usages pass in null. I believe how this should be handled would be easier to determine once we have labelled all such locations (by using the this method). If others feel that this is best done in this PR, I do not feel strongly about this, just erring on the side of less work per PR.
| } | ||
|
|
||
| public int getConnectTimeoutMs() { | ||
| return (int) getTimeoutSettings().getConnectTimeoutMS(); |
There was a problem hiding this comment.
This is because with Java sockets a timeout cannot be negative and 0 means infinite. See: socket.connect
| private final long readTimeoutMS; | ||
| // Deprecated configuration timeout options | ||
| private final long readTimeoutMS; // aka socketTimeoutMS | ||
| private final long maxWaitTimeMS; // aka waitQueueTimeoutMS |
| this(timeoutMS, null, serverSelectionTimeoutMS, connectTimeoutMS, readTimeoutMS, 0, 0, 0, null); | ||
| final long serverSelectionTimeoutMS, final long connectTimeoutMS, final long readTimeoutMS, | ||
| @Nullable final Long timeoutMS, final long maxWaitTimeMS) { | ||
| this(timeoutMS, null, serverSelectionTimeoutMS, connectTimeoutMS, readTimeoutMS, 0, 0, 0, null, maxWaitTimeMS); |
There was a problem hiding this comment.
Its needed in the constructor, so it can be copied via the with methods.
We should eventually make this constructor private or annotate visibility test only.
| @@ -41,6 +44,24 @@ public OperationContext(final RequestContext requestContext, final SessionContex | |||
| this(NEXT_ID.incrementAndGet(), requestContext, sessionContext, timeoutContext, serverApi); | |||
| } | |||
|
|
|||
|
|
|||
| public static OperationContext todoOperationContext() { | |||
| // TODO (CSOT) should be removed; used at locations that require an OC, but which do not yet have one available | |||
| public static OperationContext nonUserOperationContext(final MongoClientSettings settings) { | ||
| // TODO (CSOT) below is placeholder, validate correctness (serverApi/timeoutSettings might | ||
| // TODO (CSOT) need to be passed in instead) | ||
| TimeoutSettings timeoutSettings = TimeoutSettings.create(settings); |
| OPERATION_CONTEXT.getSessionContext(), | ||
| new TimeoutContext(new TimeoutSettings(30_000, 10_000, 0, null, 0)), | ||
| OPERATION_CONTEXT.getServerApi()); | ||
| connections.add(provider.get(operationContextWithZeroMaxWait)); |
JAVA-5212