-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(CSOT) - feature branch #4095
base: main
Are you sure you want to change the base?
Changes from 45 commits
ab21b5b
2338dff
9615138
0eb1986
69f1087
ff2ec44
a3e552a
a9c96ac
21b2fcd
892800d
6f62c9f
b878c6c
c5fae5c
124f2d7
cd6a27e
b63b63f
9f4e884
6fc1198
c18b7f0
9c40f94
a958ad8
a75d9df
875ea67
3839330
c738691
c4ae2fb
bf5a37a
e2f9125
472fa9e
43e69bd
fb96314
ad55e9c
6c7adf1
f586736
021a94d
a61a9d0
30e564c
02509a1
7c73c58
b92162b
4625af4
7d7f005
ecdd66d
9307a80
78ccbd8
c4d26c5
3590e5e
f7cc3a5
a1c7601
2ed34ac
7111909
86a7eeb
2a816e3
62bb3ca
cdce963
5c91311
72c22b9
68f1eec
e88191a
daf0d5a
1d9ac3e
e468b54
fd6c751
bab1667
4d126eb
92564d8
09a8b17
f2f4d63
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -310,7 +310,11 @@ export class Server extends TypedEventEmitter<ServerEvents> { | |||||||||||||
this.incrementOperationCount(); | ||||||||||||||
if (conn == null) { | ||||||||||||||
try { | ||||||||||||||
conn = await this.pool.checkOut(); | ||||||||||||||
if (options.operationTimeout) { | ||||||||||||||
conn = await this.pool.checkOut({ timeout: options.operationTimeout }); | ||||||||||||||
} else { | ||||||||||||||
conn = await this.pool.checkOut(); | ||||||||||||||
} | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
TS supports just calling this because the timeout is optional There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel Warren's current code is easier to read (and easier for someone editing the code later to not accidentally make the code not CSOT spec-compliant) , but if we do end up going with this suggestion can we leave a clarifying comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am surprised because breaking this up into two calls to checkOut based on a condition that does matter is more to read without more meaningful context given. Whether or not timeout exists, there is no change to how checkOut is, practically, invoked because the typescript reports that field as optional. I would actually take this further: conn = await this.pool.checkOut(options); Why do we need to make a new object here? passing through options should be fine right? Less branching paths the less there is to debug There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you elaborate on what may accidentally break the spec without a test warning us? |
||||||||||||||
if (this.loadBalanced && isPinnableCommand(cmd, session)) { | ||||||||||||||
session?.pin(conn); | ||||||||||||||
} | ||||||||||||||
|
@@ -333,6 +337,7 @@ export class Server extends TypedEventEmitter<ServerEvents> { | |||||||||||||
operationError.code === MONGODB_ERROR_CODES.Reauthenticate | ||||||||||||||
) { | ||||||||||||||
await this.pool.reauthenticate(conn); | ||||||||||||||
// TODO(NODE-5682): Implement CSOT support for socket read/write at the connection layer | ||||||||||||||
try { | ||||||||||||||
baileympearson marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
return await conn.command(ns, cmd, finalOptions, responseType); | ||||||||||||||
} catch (commandError) { | ||||||||||||||
|
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.
NOTE FOR REVIEWERS: Needed to do this to ensure that unit tests using ping work