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

RUST-807 Disallow maxPoolSize=0 #491

Merged
merged 2 commits into from
Oct 20, 2021

Conversation

abr-egn
Copy link
Contributor

@abr-egn abr-egn commented Oct 14, 2021

RUST-807

This disallows maxPoolSize=0 and merges the relevant updated spec tests for minPoolSize/maxPoolSize, skipping the one for maxPoolSize=0.

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.

So in SWIFT-1339, we decided to not implement this ticket, since we didn't already have this behavior and didn't exactly want to encourage it. What do you all think about doing that here too? cc @kmahar

@abr-egn
Copy link
Contributor Author

abr-egn commented Oct 18, 2021

So in SWIFT-1339, we decided to not implement this ticket, since we didn't already have this behavior and didn't exactly want to encourage it. What do you all think about doing that here too? cc @kmahar

The notion of an unbounded pool did make me a bit twitchy, so I'd be happier with this not being implemented.

@isabelatkinson
Copy link
Contributor

Not implementing this is alright with me. Do we need this behavior for tests?

@abr-egn
Copy link
Contributor Author

abr-egn commented Oct 18, 2021

Not implementing this is alright with me. Do we need this behavior for tests?

The tests just validate that maxPoolSize=0 is accepted, not what behavior it causes. Before this PR, as far as I can tell the driver would have accepted it and tried to execute with that as the somewhat nonsensical maximum size. I think least surprising behavior from the user's perspective would be to reject it as invalid, which would require us to skip those particular tests.

@kmahar
Copy link
Contributor

kmahar commented Oct 18, 2021

I think it's reasonable to take the same route we did in Swift and not treat zero as unlimited.

We did discuss at some point recently-ish what Rust should do on invalid/unsupported URI options and there were some points made in both directions for erroring vs. just logging a warning. I don't remember if we reached a conclusion... Regardless, it would seem we might want to apply similar logic to how we handle invalid values for valid options, such as 0 in this case.

@abr-egn abr-egn changed the title RUST-807 Treat maxPoolSize=0 as unlimited RUST-807 Disallow maxPoolSize=0 Oct 19, 2021
@abr-egn
Copy link
Contributor Author

abr-egn commented Oct 19, 2021

Updated the PR and description to disallow maxPoolSize=0; please take another look.

@abr-egn abr-egn force-pushed the RUST-807/max-pool-size branch from c138048 to 4b09480 Compare October 20, 2021 14:22
@abr-egn abr-egn merged commit 7f6f420 into mongodb:master Oct 20, 2021
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.

4 participants