perf: Skip gRPC trailers for StreamingRead & ExecuteStreamingSql by sakthivelmanii · Pull Request #3661 · googleapis/java-spanner

sakthivelmanii · 2025-02-26T04:15:08Z

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes: b/394475581

Description:
gRPC trailers are headers which is usually sent after all the messages are sent by the server in the stream. Trailers usually causes additional latency of 250-300 μs after last message is sent in the stream. From the server, we will receiving last field which tells us about last PartialResultSet and we no longer will be waiting for trailers to assume that all the messages are received.

If you write sample code, please follow the samples format.

olavloite · 2025-02-26T06:16:22Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/GrpcStreamIterator.java

  private TimeUnit streamWaitTimeoutUnit;
  private long streamWaitTimeoutValue;
  private SpannerException error;
+  private volatile boolean done;


I don't think this needs to be volatile. It will only be used by the gRPC thread that handles this stream. At least, that is if my assumption that onPartialResultSet and onCompleted are both only called by that thread.

I agree. atleast grpc threads will be accessing it sequentially not in parallel.

olavloite · 2025-02-26T06:17:07Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/GrpcResultSetTest.java

+            .addValues(Value.int64Array(longArray).toProto())
+            .setLast(false)
+            .build());
+    assertThat(resultSet.next()).isTrue();


nit: prefer the use of assertTrue and other junit assertions instead of com.google.truth.

olavloite · 2025-02-26T06:18:29Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/GrpcResultSetTest.java

+        assertThrows(
+            SpannerException.class,
+            () -> {
+              assertThat(resultSet.next()).isFalse();


How long does this take to execute?

1 nano second https://github.com/googleapis/java-spanner/blob/main/google-cloud-spanner/src/test/java/com/google/cloud/spanner/GrpcResultSetTest.java#L67

…Sql (#3661)" This reverts commit bd4b1f5.

| Package | Type | Package file | Manager | Update | Change | |---|---|---|---|---|---| | [com.google.cloud:google-cloud-spanner](https://github.com/googleapis/java-spanner) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `6.88.0` -> `6.89.0` | | [com.google.cloud:google-cloud-datastore](https://github.com/googleapis/java-datastore) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.27.0` -> `2.27.1` | | [software.amazon.awssdk:sdk-core](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.4` -> `2.31.6` | | [software.amazon.awssdk:sqs](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.4` -> `2.31.6` | | [software.amazon.awssdk:dynamodb-enhanced](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.4` -> `2.31.6` | | [software.amazon.awssdk:dynamodb](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.4` -> `2.31.6` | | [software.amazon.awssdk:aws-core](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.4` -> `2.31.6` | | [software.amazon.awssdk:bom](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.4` -> `2.31.6` | | [software.amazon.awssdk:auth](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.4` -> `2.31.6` | --- ### Release Notes <details> <summary>googleapis/java-spanner (com.google.cloud:google-cloud-spanner)</summary> ### [`v6.89.0`](https://github.com/googleapis/java-spanner/blob/HEAD/CHANGELOG.md#6890-2025-03-20) ##### Features - Enable ALTS hard bound token in DirectPath ([#3645](googleapis/java-spanner#3645)) ([42cc961](googleapis/java-spanner@42cc961)) - Next release from main branch is 6.89.0 ([#3669](googleapis/java-spanner#3669)) ([7a8a29b](googleapis/java-spanner@7a8a29b)) - Support isolation level REPEATABLE_READ for R/W transactions ([#3670](googleapis/java-spanner#3670)) ([e62f5ab](googleapis/java-spanner@e62f5ab)) ##### Bug Fixes - **deps:** Update the Java code generator (gapic-generator-java) to 2.55.1 ([b959f4c](googleapis/java-spanner@b959f4c)) - Revert the ALTS bound token enablement ([#3679](googleapis/java-spanner#3679)) ([183c1f0](googleapis/java-spanner@183c1f0)) ##### Performance Improvements - Get database dialect using multiplexed session ([#3684](googleapis/java-spanner#3684)) ([f641a40](googleapis/java-spanner@f641a40)) - Skip gRPC trailers for StreamingRead & ExecuteStreamingSql ([#3661](googleapis/java-spanner#3661)) ([bd4b1f5](googleapis/java-spanner@bd4b1f5)) ##### Dependencies - Update dependency com.google.cloud:sdk-platform-java-config to v3.45.1 ([#3689](googleapis/java-spanner#3689)) ([67188df](googleapis/java-spanner@67188df)) </details> <details> <summary>googleapis/java-datastore (com.google.cloud:google-cloud-datastore)</summary> ### [`v2.27.1`](https://github.com/googleapis/java-datastore/blob/HEAD/CHANGELOG.md#2271-2025-03-18) ##### Bug Fixes - **deps:** Update the Java code generator (gapic-generator-java) to 2.55.1 ([ba1ad98](googleapis/java-datastore@ba1ad98)) ##### Dependencies - Update dependency com.google.cloud:sdk-platform-java-config to v3.45.1 ([#1791](googleapis/java-datastore#1791)) ([ab5ac8e](googleapis/java-datastore@ab5ac8e)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 6pm every weekday,before 2am every weekday" in timezone Australia/Melbourne, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Never, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). GitOrigin-RevId: 5b5b3e1fd7177ff56a483be663b20b28d243550c

sakthivelmanii requested a review from a team February 26, 2025 04:15

sakthivelmanii requested a review from a team as a code owner February 26, 2025 04:15

product-auto-label bot added size: m Pull request size is medium. api: spanner Issues related to the googleapis/java-spanner API. labels Feb 26, 2025

sakthivelmanii self-assigned this Feb 26, 2025

olavloite reviewed Feb 26, 2025

View reviewed changes

sakthivelmanii force-pushed the skip_grpc_trailers_in_java branch from 977534f to a2687ea Compare February 26, 2025 06:24

sakthivelmanii changed the title ~~fix(spanner): Skip gRPC trailers for StreamingRead & ExecuteStreamingSql~~ perf: Skip gRPC trailers for StreamingRead & ExecuteStreamingSql Feb 26, 2025

sakthivelmanii force-pushed the skip_grpc_trailers_in_java branch 2 times, most recently from 524bfb2 to 83534f6 Compare February 26, 2025 06:40

sakthivelmanii added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 2, 2025

perf: Skip gRPC trailers for StreamingRead & ExecuteStreamingSql

0f5c90c

sakthivelmanii force-pushed the skip_grpc_trailers_in_java branch from 83534f6 to 0f5c90c Compare March 17, 2025 08:15

sakthivelmanii removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 17, 2025

olavloite approved these changes Mar 17, 2025

View reviewed changes

sakthivelmanii merged commit bd4b1f5 into main Mar 17, 2025
35 of 36 checks passed

sakthivelmanii deleted the skip_grpc_trailers_in_java branch March 17, 2025 13:35

release-please bot mentioned this pull request Mar 17, 2025

chore(main): release 6.89.0 #3685

Merged

sakthivelmanii added a commit that referenced this pull request Mar 18, 2025

Revert "perf: Skip gRPC trailers for StreamingRead & ExecuteStreaming…

9c9e156

…Sql (#3661)" This reverts commit bd4b1f5.

suztomo mentioned this pull request Mar 20, 2025

protobuf ci 4.29.x #3703

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: Skip gRPC trailers for StreamingRead & ExecuteStreamingSql#3661

perf: Skip gRPC trailers for StreamingRead & ExecuteStreamingSql#3661
sakthivelmanii merged 1 commit intomainfrom
skip_grpc_trailers_in_java

sakthivelmanii commented Feb 26, 2025 •

edited

Loading

Uh oh!

olavloite Feb 26, 2025

Uh oh!

sakthivelmanii Feb 26, 2025

Uh oh!

olavloite Feb 26, 2025

Uh oh!

sakthivelmanii Feb 26, 2025

Uh oh!

olavloite Feb 26, 2025

Uh oh!

sakthivelmanii Feb 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

sakthivelmanii commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olavloite Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

sakthivelmanii Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

olavloite Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

sakthivelmanii Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

olavloite Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

sakthivelmanii Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sakthivelmanii commented Feb 26, 2025 •

edited

Loading