-
Notifications
You must be signed in to change notification settings - Fork 106
Pin Servers during Sharded Transactions #411
Conversation
dc3ef29
to
e4f1af7
Compare
6bd55c7
to
9a63109
Compare
9a63109
to
8e0c9f9
Compare
a466b0c
to
5db3c1b
Compare
5db3c1b
to
01a2fa8
Compare
204d313
to
dc02410
Compare
03de14a
to
2ae6b4f
Compare
const session = options.session; | ||
const transaction = session && session.transaction; | ||
|
||
if (isSharded && transaction && transaction.server) { |
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.
mongos.js
has a protection here to ensure that the pinned server is actually connected. Should we do that here?
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.
I think that's because the old mongos doesn't have proper server selection. Anyway, I think it's too defensive to check here and I'd rather fix this holistically in our new topology if the issue comes up.
lib/transactions.js
Outdated
return; | ||
} | ||
|
||
throw new MongoError( | ||
`Attempted illegal state transition from [${this.state}] to [${nextState}]` | ||
); | ||
} | ||
|
||
get isPinned() { |
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.
it looks like get isPinned
is defined twice here, and not actually used.
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.
whoops, removing
Changes to ensure that all commands in a sharded transaction run against a single
mongos
.Items to still do: