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

DOCSP-47031 Connection Pools #620

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

lindseymoore
Copy link
Collaborator

@lindseymoore lindseymoore commented Feb 20, 2025

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-47031

Staging Links

  • connection/connection-pools
  • connection
  • Self-Review Checklist

    • Is this free of any warnings or errors in the RST?
    • Did you run a spell-check?
    • Did you run a grammar-check?
    • Are all the links working?
    • Are the facets and meta keywords accurate?

    Copy link

    netlify bot commented Feb 20, 2025

    Deploy Preview for docs-java ready!

    Name Link
    🔨 Latest commit 7bd7662
    🔍 Latest deploy log https://app.netlify.com/sites/docs-java/deploys/67c1e072110db00008592943
    😎 Deploy Preview https://deploy-preview-620--docs-java.netlify.app
    📱 Preview on mobile
    Toggle QR Code...

    QR Code

    Use your smartphone camera to open QR code link.

    To edit notification comments on pull requests, go to your Netlify site configuration.

    @lindseymoore lindseymoore marked this pull request as draft February 21, 2025 16:10
    Copy link
    Contributor

    @rachel-mack rachel-mack left a comment

    Choose a reason for hiding this comment

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

    A few tweaks:

    Comment on lines 54 to 56
    The following code creates a client with a maximum connection pool size of ``50``
    by using either a :guilabel:`Connection String` or :guilabel:`MongoClientSettings`
    object:
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    I haven't seen :guilabel: used before? I guess it just bold the text?

    Anyway, would make these links to the Connection Options and MongoClient Settings pages.

    Copy link
    Collaborator Author

    @lindseymoore lindseymoore Feb 24, 2025

    Choose a reason for hiding this comment

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

    @rachel-mack Thanks for the below comments! The links to these pages can be found in the tabs. Since the links are now in a more content relevant place in the tabs, I think leaving without a link here is appropriate.

    guilabel can be used when referring to UI components on the page (such as the tabs) or in the Atlas UI. Here are a few examples I've seen:

    Updated the copy to specifically mention the tabs.

    @lindseymoore lindseymoore marked this pull request as ready for review February 24, 2025 22:46
    @lindseymoore lindseymoore requested a review from a team as a code owner February 24, 2025 22:46
    @lindseymoore lindseymoore requested review from jyemin and removed request for a team February 24, 2025 22:46
    Copy link
    Collaborator

    @jyemin jyemin left a comment

    Choose a reason for hiding this comment

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

    In general there is not much guidance here on why you would set any of these properties to a non-default value, but if we're not trying to explain that, than I can accept it. It would certainly require a lot more thought, as there are no strict rules to follow.

    * - Setting
    - Description

    * - ``connectTimeoutMS``
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    connectTimeoutMS and socketTimeoutMS are considered socket settings, not connection pool settings, and are configured programmatically not via ConnectionPoolSettings but via SocketSettings.

    It's ok to include them on the same page but they should be separated, and we should show how to configure them programmatically as well.

    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    Thanks for the context, I can remove them from the table for simplicity's sake.


    - Specifies the maximum amount of time, in milliseconds, the Java driver
    waits for a connection to open before timing out. A value of 0 instructs
    the driver to never time out while waiting for a connection to open.
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    This is true, but in practice the OS has a connect timeout so it won't really wait forever. See if you can find a reference for that somewhere?

    Copy link
    Collaborator Author

    @lindseymoore lindseymoore Feb 27, 2025

    Choose a reason for hiding this comment

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

    Removed from the option instead, due to previous comment.

    @lindseymoore lindseymoore requested a review from jyemin February 27, 2025 23:54
    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