Skip to content
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

Fix csot test failures #2853

Merged
merged 14 commits into from
Apr 2, 2024

Conversation

comandeo-mongo
Copy link
Contributor

No description provided.

@comandeo-mongo comandeo-mongo marked this pull request as ready for review March 27, 2024 16:52
@comandeo-mongo comandeo-mongo requested a review from jamis March 27, 2024 16:52
Copy link
Contributor

@jamis jamis left a comment

Choose a reason for hiding this comment

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

Great job, I'm really happy to see the switch to IO.select versus Timeout.timeout. I'm not adamant about the duplicated code--I can imagine how it might be a little tricky to refactor that out, and if it is, then don't worry about it.

Otherwise, LGTM! 👍

else
retry
end
retry
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a stray retry here. Also, feels like these two rescue blocks have a lot in common (differ only on whether the sockets are waiting for read or write). Could the logic be pulled out somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thank you. Extracted the retry logic in a separate method.

@comandeo-mongo comandeo-mongo requested a review from jamis March 28, 2024 11:36
@comandeo-mongo comandeo-mongo merged commit d5df239 into mongodb:csot Apr 2, 2024
313 of 330 checks passed
@comandeo-mongo comandeo-mongo deleted the fix-csot-test-failures branch April 2, 2024 08:10
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.

2 participants