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-1231 Run tests using DEFAULT_MAX_POOL_SIZE #980

Merged
merged 3 commits into from
Oct 31, 2023

Conversation

stIncMale
Copy link
Member

@stIncMale stIncMale commented Oct 23, 2023

The tasks compile-on-msrv and check-cargo-deny fail because of RUST-1780.

RUST-1231

@abr-egn abr-egn requested review from isabelatkinson and removed request for patrickfreed October 26, 2023 17:51
@abr-egn
Copy link
Contributor

abr-egn commented Oct 26, 2023

Swapped Patrick out for Isabel since he's no longer on the Rust driver team :)

Copy link
Contributor

@abr-egn abr-egn left a comment

Choose a reason for hiding this comment

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

LGTM!

@stIncMale
Copy link
Member Author

Merged main to make all tests pass.

src/test.rs Outdated
@@ -114,7 +113,7 @@ lazy_static! {

pub(crate) fn update_options_for_testing(options: &mut ClientOptions) {
if options.max_pool_size.is_none() {
options.max_pool_size = Some(MAX_POOL_SIZE);
options.max_pool_size = Some(DEFAULT_MAX_POOL_SIZE);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this logic entirely since the driver will already apply this default internally.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed! Done.

@@ -216,7 +217,7 @@ async fn load_balancing_test() {
let mut handler = EventHandler::new();
let mut subscriber = handler.subscribe();
let mut options = CLIENT_OPTIONS.get().await.clone();
let max_pool_size = 10;
let max_pool_size = DEFAULT_MAX_POOL_SIZE;
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto here, I think we just want min/max pool size to be the same, so line 224 can just assign to Some(DEFAULT_MAX_POOL_SIZE)

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed options.max_pool_size = Some(max_pool_size), but the max_pool_size variable is still needed nonetheless (it seems better to use it in multiple places in the method, rather than using DEFAULT_MAX_POOL_SIZE in those same places, and it is how it was done originally).

Don't implicitly set `max_pool_size` to `DEFAULT_MAX_POOL_SIZE`.

RUST-1231
@stIncMale
Copy link
Member Author

@isabelatkinson Could you please also merge the PR? Asking because I don't have the permission to do that.

@isabelatkinson isabelatkinson merged commit 6d4e316 into mongodb:main Oct 31, 2023
@stIncMale stIncMale deleted the RUST-1231 branch October 31, 2023 19:29
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