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
Merged

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 as code owners Jun 1, 2021
@product-auto-label product-auto-label bot added the api: spanner label Jun 1, 2021
@google-cla google-cla bot added the cla: yes label Jun 1, 2021
@nimf nimf marked this pull request as draft Jun 1, 2021
@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 issue 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 Jun 13, 2021
@olavloite
Copy link
Contributor

@olavloite olavloite commented Jun 14, 2021

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

@thiagotnunes thiagotnunes Jun 14, 2021

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

@nimf nimf Jun 16, 2021

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

@thiagotnunes thiagotnunes Jun 21, 2021

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
Contributor

@olavloite olavloite commented Jun 15, 2021

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 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
Contributor

@olavloite olavloite 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

@google-cla
Copy link

@google-cla 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 and removed cla: yes labels Jun 23, 2021
@nimf
Copy link
Contributor Author

@nimf 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 issue 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 and removed cla: no labels Jun 24, 2021
@olavloite
Copy link
Contributor

@olavloite olavloite commented Jun 24, 2021

@googlebot I consent.

Copy link
Contributor

@olavloite olavloite left a comment

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 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 label Jun 29, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Jun 29, 2021
@thiagotnunes thiagotnunes merged commit 1fa95a9 into googleapis:master Jun 29, 2021
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants