Skip to content

CSOT: Added timeout to settings & connection string #1173

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

Merged
merged 0 commits into from
Aug 22, 2023

Conversation

rozza
Copy link
Member

@rozza rozza commented Aug 10, 2023

@rozza rozza requested a review from katcharov August 10, 2023 14:13
@rozza
Copy link
Member Author

rozza commented Aug 10, 2023

Note: I tried to remain consistent with the rest of the code. So added the MS suffix where appropriate. Also in the legacy driver the API for timeouts returns milliseconds and doesn't take a TimeUnit.

Copy link
Collaborator

@katcharov katcharov left a comment

Choose a reason for hiding this comment

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

LGTM, and a question

@@ -280,6 +282,7 @@ public class ConnectionString {
private Integer maxConnectionLifeTime;
private Integer maxConnecting;
private Integer connectTimeout;
private Long timeout;
Copy link
Collaborator

@katcharov katcharov Aug 16, 2023

Choose a reason for hiding this comment

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

should this have the unit, timeoutMS?

Perhaps it is ok if this is inconsistent with the others, since they are deprecated, and this is consistent with the suffixing elsewhere? In any case, the docs on the method should probably use the term "milliseconds".

Copy link
Member Author

Choose a reason for hiding this comment

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

Added to the javadocs its milliseconds. Its annoying that its inconsistent but I opted to keep it consistently inconsistent as not all timeouts will be deprecated.

@rozza rozza changed the base branch from master to CSOT August 22, 2023 10:27
@rozza rozza merged this pull request into mongodb:CSOT Aug 22, 2023
rozza added a commit to rozza/mongo-java-driver that referenced this pull request Aug 22, 2023
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