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

CASSGO-3 detailed description for NumConns #1821

Conversation

OleksiienkoMykyta
Copy link
Contributor

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.

@joao-r-reis joao-r-reis changed the title detailed description for NumConns CASSGO-3 detailed description for NumConns Oct 29, 2024
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.
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Check this and this.

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

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@OleksiienkoMykyta OleksiienkoMykyta force-pushed the issue-1741-cant-limit-number-of-connections-per-pod branch 2 times, most recently from f7bdc05 to 8ebc42d Compare November 15, 2024 14:14
@OleksiienkoMykyta OleksiienkoMykyta force-pushed the issue-1741-cant-limit-number-of-connections-per-pod branch from 8ebc42d to ad35033 Compare November 22, 2024 11:25
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
@OleksiienkoMykyta OleksiienkoMykyta force-pushed the issue-1741-cant-limit-number-of-connections-per-pod branch from ad35033 to 35d9ca4 Compare November 22, 2024 11:29
@joao-r-reis joao-r-reis merged commit 280f0aa into apache:trunk Nov 22, 2024
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.

CASSGO-3 How to set limit for pool connection
3 participants