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

feat(spanner): Add option to disable closing spanner clients #910

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jtwatson
Copy link

@jtwatson jtwatson commented Apr 2, 2023

Provide an option to prevent the provided *spanner.Client and *database.DatabaseAdminClient from being closed when migrate closes Spanner

Spanner does not have the same concepts as PostgreSQL and MySQL, where it makes sense to use a WithConnection() function as the signature would be identical to that of WithInstance(). It could appear that they are interchangeable when really one closes the connection, and the other does not.

I chose to take a simple path of adding an option to the Config, DoNotCloseSpannerClients. This is a very simple change that is clear about its effect and does not change the default behavior of WithInstance().

But this is not in line with the other drivers implementing this same feature, so feels a little inconsistent.

Maybe someone has a better idea.

See the following thread for reference:

@coveralls
Copy link

coveralls commented Apr 2, 2023

Coverage Status

coverage: 59.21% (-0.02%) from 59.226%
when pulling 9c48518 on jtwatson:feature/spanner-client-reuse
into 0d41589 on golang-migrate:master.

@jtwatson jtwatson force-pushed the feature/spanner-client-reuse branch from 38a4684 to e011fdd Compare May 7, 2023 02:27
@jtwatson jtwatson force-pushed the feature/spanner-client-reuse branch from e011fdd to 1b30f7d Compare July 27, 2023 03:52
@jtwatson jtwatson force-pushed the feature/spanner-client-reuse branch from 1b30f7d to 9c48518 Compare January 4, 2024 04:35
@jtwatson
Copy link
Author

jtwatson commented Jan 4, 2024

@dhui @Fontinalis, can you take a look at this PR? I am hoping to get some feedback on the provided feature.

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.

None yet

2 participants