-
Notifications
You must be signed in to change notification settings - Fork 623
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
CASSGO-3 detailed description for NumConns #1821
CASSGO-3 detailed description for NumConns #1821
Conversation
cluster.go
Outdated
@@ -102,7 +102,8 @@ type ClusterConfig struct { | |||
// Initial keyspace. Optional. | |||
Keyspace string | |||
|
|||
// Number of connections per host. | |||
// The size of connection pool for each host. The Pool will be filled during the first request execution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is 100% correct, if InitialHostLookup
is enabled (it's the default) then the pools for the discovered hosts will be filled during session initialization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right!
I double-checked it with the timeout, and it turned out that the pool filled during initialization. When I tested it last time the pool had not been filled before the query execution because pool filling runs in separate gorutine and it took more time to finish than the query execution.
Thank you for noticing the issue!
I can replace it with something like
The size of the connection pool for each host. The pool filling runs in separate gourutine during the session initialization phase.
Also, it describes a maximum number of connections at the same time.
Notice: There is no guarantee that pool filling will be finished in the initialization phase.
Default: 2
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm interesting, did you use SingleHostReadyPolicy
? If you don't use this policy then it should wait until all hosts have at least 1 connection in the pool before the session initialization is done.
Considering this, it would probably be good to mention that gocql will always try to get 1 connection on each host pool during session initialization AND it will attempt to fill each pool afterwards asynchronously if NumConns > 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about something like that?
// The size of the connection pool for each host.
// The pool filling runs in separate gourutine during the session initialization phase.
// gocql will always try to get 1 connection on each host pool
// during session initialization AND it will attempt
// to fill each pool afterward asynchronously if NumConns > 1.
// Notice: There is no guarantee that pool filling will be finished in the initialization phase.
// Also, it describes a maximum number of connections at the same time.
I have added gocql will always try to get 1 connection on each host pool during session initialization AND it will attempt to fill each pool afterward asynchronously if NumConns > 1
as you suggested, so there is no more room for unclearness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
f7bdc05
to
8ebc42d
Compare
8ebc42d
to
ad35033
Compare
NumConns doesn`t have a proper description, so it could cause misunderstanding and confusion about this option. patch by Mykyta Oleksiienko; reviewed by Joao Reis and Jackson Fleming for CASSGO-3
ad35033
to
35d9ca4
Compare
Closes #1741 .
There is some unclearness in the NumConns description because there are no details about what it affects, and does it sets any limits. I added a few more details to avoid confusion about it in the future.