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

Explicitly state max_connections and timeout defaults in docs #688

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

sbinlondon
Copy link
Contributor

There's no explicitly stated defaults (that I can find) for max_connections and timeout.

The README examples, if they are examples of the defaults, say 100 for max_connections but then in hackney.app.src it seems to be 50? I went with that one but if it is something else, please let me know and happy to update my PR.

Either way it would be helpful to have very clearly stated defaults.

Copy link
Owner

@benoitc benoitc left a comment

Choose a reason for hiding this comment

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

thanks for the changes, this is very helpful. Can you add also the unit to the timeout ?

README.md Outdated
@@ -368,7 +368,7 @@ ok = hackney_pool:start_pool(PoolName, Options),
`timeout` is the time we keep the connection alive in the pool,
`max_connections` is the number of connections maintained in the pool. Each
connection in a pool is monitored and closed connections are removed
automatically.
automatically. The default timeout is `150000` and the default number of connections is `50`.
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add the unit to the timeout? ie. 150000 ms sot it would definitely clarify the text

@sbinlondon
Copy link
Contributor Author

@benoitc let me know if that new formatting is ok or if you want any more changes.

@benoitc benoitc merged commit 870b6ad into benoitc:master Oct 10, 2023
@benoitc
Copy link
Owner

benoitc commented Oct 10, 2023

finally merged. Thanks!

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