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

chore: Replace channel provider for fixed channel provider. #3582

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

Conversation

nimf
Copy link
Contributor

@nimf nimf commented Jan 3, 2025

Currently, when creating a stub, the channel provider creates a new channel pool every time, thus we end up with four channel pools to the same target instead of one.

In this PR we create a single channel pool using the existing channel provider and then wrap it into FixedTransportChannelProvider so that any stub using the fixed channel provider uses the created pool instead of creating a new one.

@surbhigarg92 do I also need to change anything in the metrics according to this change?

Create a single channel pool using the existing channel provider and then wrap it into FixedTransportChannelProvider so that any stub using the fixed channel provider uses the created pool instead of creating a new one every time a stub is created.
@nimf nimf requested a review from a team as a code owner January 3, 2025 20:28
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: spanner Issues related to the googleapis/java-spanner API. labels Jan 3, 2025
@rahul2393 rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 6, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 6, 2025
Comment on lines +406 to +407
FixedTransportChannelProvider fixedChannelProvider =
FixedTransportChannelProvider.create(channelProvider.getTransportChannel());
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that we need a bit more management and logic around this:

  1. We should only use a FixedTransportChannelProvider if the user has not specified a channel provider themselves. That is; We should only do this if (options.getChannelProvider() == null). The reason is that for the default channel provider, we know that it produces a channel pool, which can safely be wrapped in a FixedChannelProvider. We don't know what any custom channel provider that has been set might produce, and whether it is a good idea to just get one channel from that provider and use that for all requests.
  2. We also need to keep a reference to the channel that is returned by defaultChannelProvider.build().getTransportChannel(), and then close that channel when this GapicSpannerRpc instance is shut down. A FixedTransportChannelProvider is not closed when a stub is closed. This will leave the channels open after this instance has been closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/java-spanner API. size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants