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

Introduce initial reconnection policy #231

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

dkropachev
Copy link
Collaborator

@dkropachev dkropachev commented Aug 7, 2024

We would want to retry on initial connection to the cluster, in case if cluster is not ready yet.

This PR introduced new InitialReconnectionPolicy option to ServerConfig, by default it is NoReconnectionPolicy, to ensure old behavior.

Closes #230

@dkropachev dkropachev linked an issue Aug 7, 2024 that may be closed by this pull request
@dkropachev dkropachev force-pushed the dk/230-initial-connection-vs-reconnections branch from da3daed to d51b3dc Compare August 7, 2024 12:44
@dkropachev dkropachev force-pushed the dk/230-initial-connection-vs-reconnections branch 2 times, most recently from 28c1ae2 to 270ba1e Compare August 7, 2024 12:59
@dkropachev dkropachev self-assigned this Aug 7, 2024
@knadh
Copy link

knadh commented Aug 7, 2024

This is good to see! A couple of thoughts:

The ideal behaviour would be for the reconnect attempts to happen in the background or to be deferred (this could be configurable in the policy) so that NewSession() does not block. In many production scenarios, an app should be able to boot without a single DB driver blocking indefinitely, and the drive connects whenever the upstream server becomes available based on reconnection policies.

That's how Postgres, Redis etc. drivers/clients generally behave, where the client can be initialized without blocking, and the actual connection and discovery happen based on activity (Ping() or Query() etc.) or in the background. Any attempt to use the client without an underlying connection simply returns an error without panicking, which can be handled gracefully.

Thank you.

@dkropachev
Copy link
Collaborator Author

dkropachev commented Aug 7, 2024

This is good to see! A couple of thoughts:

The ideal behaviour would be for the reconnect attempts to happen in the background or to be deferred (this could be configurable in the policy) so that NewSession() does not block. In many production scenarios, an app should be able to boot without a single DB driver blocking indefinitely, and the drive connects whenever the upstream server becomes available based on reconnection policies.

That's how Postgres, Redis etc. drivers/clients generally behave, where the client can be initialized without blocking, and the actual connection and discovery happen based on activity (Ping() or Query() etc.) or in the background. Any attempt to use the client without an underlying connection simply returns an error without panicking, which can be handled gracefully.

Thank you.

Thanks for taking your time reviewing it and commenting.
It is better to do that in a seperate PR, it implicate totally different way of initialization and some scenarios become invalid anymore, for example currently we are checking if dc in load balancing policy matches cluster configuration and throw an error if it is not.
Such validations are imposible until driver is actually connected.

@sylwiaszunejko
Copy link
Collaborator

Do we want to check the behavior with different InitialReconnectionPolicy than default? I am wondering if there is some way to test it?

@dkropachev dkropachev force-pushed the dk/230-initial-connection-vs-reconnections branch from 270ba1e to 24912ab Compare August 8, 2024 14:05
@dkropachev
Copy link
Collaborator Author

Do we want to check the behavior with different InitialReconnectionPolicy than default? I am wondering if there is some way to test it?

Done, please take a look

@dkropachev
Copy link
Collaborator Author

This is good to see! A couple of thoughts:
The ideal behaviour would be for the reconnect attempts to happen in the background or to be deferred (this could be configurable in the policy) so that NewSession() does not block. In many production scenarios, an app should be able to boot without a single DB driver blocking indefinitely, and the drive connects whenever the upstream server becomes available based on reconnection policies.
That's how Postgres, Redis etc. drivers/clients generally behave, where the client can be initialized without blocking, and the actual connection and discovery happen based on activity (Ping() or Query() etc.) or in the background. Any attempt to use the client without an underlying connection simply returns an error without panicking, which can be handled gracefully.
Thank you.

Thanks for taking your time reviewing it and commenting. It is better to do that in a seperate PR, it implicate totally different way of initialization and some scenarios become invalid anymore, for example currently we are checking if dc in load balancing policy matches cluster configuration and throw an error if it is not. Such validations are imposible until driver is actually connected.

@knadh , please take a look at #234

@dkropachev dkropachev merged commit c2a9055 into master Aug 26, 2024
1 check passed
@dkropachev dkropachev deleted the dk/230-initial-connection-vs-reconnections branch August 26, 2024 11:27
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.

Initial connection vs. reconnections
3 participants