Skip to content

Conversation

@kmahar
Copy link
Contributor

@kmahar kmahar commented Feb 4, 2021

Fixes the transactions test failures by using separate clients for running specific commands against each mongos. As discussed in #driver-devs, this accounts for cases where the setup client has not yet reconnected to one of the mongoses after a test.

I thought about porting this over to the unified runner, but that one is fairly specific that the internal client should be used for terminating open transactions and doesn't appear to be suffering from this issue on its own. It may become necessary in the future if any similarly racy tests start using that runner.

Evg patch against all sharded variants (realized those < 4.2 were useless too late): https://evergreen.mongodb.com/version/601c817c850e612ab835c3c9

no macOS results in yet but as you can see this has fixed all the Ubuntu failures anyway.

@kmahar kmahar requested a review from patrickfreed February 4, 2021 23:48
@kmahar
Copy link
Contributor Author

kmahar commented Feb 5, 2021

update: there were a few macOS failures but they are all due to the libmongoc crashing on sigpipe issue so those will be fixed by vendoring in 1.17.4.

Copy link
Contributor

@patrickfreed patrickfreed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one nit/suggestion but otherwise LGTM (don't need to review again).

great job hunting down these failures!

// Due to strange behavior in mongos, a "distinct" command needs to be run against each mongos
// before the tests run to prevent certain errors from ocurring. (SERVER-39704)
if MongoSwiftTestCase.topologyType == .sharded,
if [.sharded, .shardedReplicaSet].contains(topologyType),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this will likely be quite common, could we add an isSharded (or something similar) method to TopologyType?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice idea, done ✅

@kmahar kmahar merged commit 917e177 into master Feb 5, 2021
@kmahar kmahar deleted the fix-txn-tests branch February 5, 2021 03:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants