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: add gRPC-GCP channel pool as an option #1227

Merged
merged 18 commits into from Jun 29, 2021

Conversation

nimf
Copy link
Contributor

@nimf nimf commented Jun 1, 2021

This enables a user to opt-in for using the gRPC-GCP channel pool and configure its options.

For example:

    SpannerOptions.Builder optionsBuilder = SpannerOptions.newBuilder();

    // ...

    optionsBuilder.withGrpcGcpOptions(GcpManagedChannelOptions.newBuilder()
        .withResiliencyOptions(GcpResiliencyOptions.newBuilder()
            .setNotReadyFallback(true)
            .build())
        .build());

    Spanner spanner = optionsBuilder.build().getService();

The gRPC-GCP channel pool provides a couple resiliency improvements during some networking issues. It also provides additional channel pool metrics.

@nimf nimf requested review from a team as code owners June 1, 2021 19:40
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Jun 1, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 1, 2021
@nimf nimf marked this pull request as draft June 1, 2021 19:42
@nimf nimf changed the title Add gRPC-GCP channel pool as an option feat: add gRPC-GCP channel pool as an option Jun 1, 2021
thiagotnunes pushed a commit to thiagotnunes/java-spanner that referenced this pull request Jun 5, 2021
# Conflicts:
#	google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java
#	google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java
@nimf nimf marked this pull request as ready for review June 13, 2021 19:39
@olavloite
Copy link
Collaborator

In order to fix the failed lint check you need to run the following command:

mvn com.coveo:fmt-maven-plugin:format

<dependency>
<groupId>com.google.cloud</groupId>
<artifactId>grpc-gcp</artifactId>
<version>1.0.0</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

We tend to use the versions specified in java-shared-dependencies. By that, I mean we usually do NOT specify version dependencies (like here), but instead rely on the versions provided by the POM import of that repository.

Could we do the same for this artifact (if it is part of any BOMs specified in the shared dependencies)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not a part of any BOMs. If I add it to the java-shared-dependencies will all the projects depending on it automatically get grpc-gcp as a dependency or they must explicitly add it to their dependencies?

Copy link
Contributor

Choose a reason for hiding this comment

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

They must explicitly add it to their dependencies, but they won't need to specify a version. This makes sure that we are always using compatible library versions.

@olavloite
Copy link
Collaborator

Also, could we have a couple of tests for this feature that:

  1. Verifies that the creation of sessions is actually spread across multiple channels. See SessionClientTest for the current tests for the non-gRPC-GCP channel pool solution.
  2. An end-to-end test that enables this option and executes a couple of transactions to verify that enabling this option works (and continues to work in the future).

@nimf
Copy link
Contributor Author

nimf commented Jun 16, 2021

Also, could we have a couple of tests for this feature that:

  1. An end-to-end test that enables this option and executes a couple of transactions to verify that enabling this option works (and continues to work in the future).

Added.

  1. Verifies that the creation of sessions is actually spread across multiple channels. See SessionClientTest for the current tests for the non-gRPC-GCP channel pool solution.

For the GAX channel pool it is important which CHANNEL_HINT values the Spanner client uses so that every BatchCreateSession request uses different channel. And the SessionClientTest currently verifies that correct channel hints values are used. But the gRPC-GCP channel pool doesn't use channel hints. Instead these are the things required from the Spanner client to spread the sessions correctly:

  • Issue N+ BatchCreateSession requests (where N is the number of channels) (this is probably already tested)
  • Set up GAX with the pool size of 1
  • Set up low watermark for gRPC-GCP to 0

Can you give me a hint on how it is easier to test the last two, please?

@olavloite
Copy link
Collaborator

@nimf I can't push any changes to your branch, so I'll attach a proposal for a test as an attachment here. Please feel free to make any changes you think would be appropriate to the test.
ChannelUsageTest.java.txt

@google-cla
Copy link

google-cla bot commented Jun 23, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Jun 23, 2021
@nimf
Copy link
Contributor Author

nimf commented Jun 23, 2021

@nimf I can't push any changes to your branch, so I'll attach a proposal for a test as an attachment here. Please feel free to make any changes you think would be appropriate to the test.
ChannelUsageTest.java.txt

Wow, it's perfect! Thank you so much!

I've added you as the author, but now it requires a consent. Can you give your consent, please?

gcf-merge-on-green bot pushed a commit to googleapis/java-shared-dependencies that referenced this pull request Jun 23, 2021
Add gRPC-GCP extension library to shared dependencies.

This will be used by the Spanner client first: googleapis/java-spanner#1227
@thiagotnunes thiagotnunes added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jun 24, 2021
@olavloite
Copy link
Collaborator

@googlebot I consent.

Copy link
Collaborator

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

LGTM

I assume the 1.0.0 version for the grpc-gcp in the pom.xml can be removed once the shared dependencies with this dependency included has been released (?)

@nimf
Copy link
Contributor Author

nimf commented Jun 28, 2021

I assume the 1.0.0 version for the grpc-gcp in the pom.xml can be removed once the shared dependencies with this dependency included has been released (?)

Yes, exactly. I've already added grpc-gcp to the java-shared-dependencies but it hasn't been released yet.

@thiagotnunes thiagotnunes added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 29, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 29, 2021
@thiagotnunes thiagotnunes merged commit 1fa95a9 into googleapis:master Jun 29, 2021
suztomo pushed a commit to googleapis/sdk-platform-java that referenced this pull request Mar 21, 2023
Add gRPC-GCP extension library to shared dependencies.

This will be used by the Spanner client first: googleapis/java-spanner#1227
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. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants