Skip to content

chore(x-goog-spanner-request-id): plumb for BatchCreateSessions #3815

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 22 commits into from
May 26, 2025
Merged

chore(x-goog-spanner-request-id): plumb for BatchCreateSessions #3815

merged 22 commits into from
May 26, 2025

Conversation

odeke-em
Copy link
Contributor

This change plumbs x-goog-spanner-request-id into BatchCreateSessions and asserts that the header is present for that method.

Updates #3537

@odeke-em odeke-em requested review from a team as code owners April 10, 2025 19:59
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: spanner Issues related to the googleapis/java-spanner API. labels Apr 10, 2025
@odeke-em odeke-em force-pushed the x-goog-spanner-request-id-plumb-into-BatchCreateSessions branch 2 times, most recently from b9e9734 to f4c6768 Compare April 10, 2025 20:09
@odeke-em odeke-em mentioned this pull request Apr 11, 2025
Closed
@odeke-em odeke-em force-pushed the x-goog-spanner-request-id-plumb-into-BatchCreateSessions branch from f4c6768 to ecb64c7 Compare April 11, 2025 17:55
@odeke-em
Copy link
Contributor Author

@sakthivelmanii @olavloite @rahul2393 @surbhigarg92 kindly help me run the bots here. Thank you.

@odeke-em odeke-em force-pushed the x-goog-spanner-request-id-plumb-into-BatchCreateSessions branch 2 times, most recently from 4bdb739 to e7463fb Compare April 16, 2025 18:25
@rahul2393 rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 17, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 17, 2025
olavloite
olavloite reviewed May 14, 2025
View reviewed changes
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionImpl.java
@@ -435,14 +466,18 @@ public AsyncTransactionManagerImpl transactionManagerAsync(TransactionOption...

@Override
public ApiFuture<Empty> asyncClose() {
return spanner.getRpc().asyncDeleteSession(getName(), getOptions());
XGoogSpannerRequestId reqId =
this.getRequestIdCreator().nextRequestId(1 /* TODO: channelId */, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
this.getRequestIdCreator().nextRequestId(1 /* TODO: channelId */, 0);
this.getRequestIdCreator().nextRequestId(1 /* TODO: channelId */, 1);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I manually have been increasing attempts for it. Let's get this merged in as is and in the next round I shall get them all fixed up.

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionImpl.java
}

@Override
public void close() {
ISpan span = tracer.spanBuilder(SpannerImpl.DELETE_SESSION);
try (IScope s = tracer.withSpan(span)) {
spanner.getRpc().deleteSession(getName(), getOptions());
XGoogSpannerRequestId reqId =
this.getRequestIdCreator().nextRequestId(1 /* TODO: channelId */, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
this.getRequestIdCreator().nextRequestId(1 /* TODO: channelId */, 0);
this.getRequestIdCreator().nextRequestId(1 /* TODO: channelId */, 1);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I manually have been increasing attempts for it. Let's get this merged in as is and in the next round I shall get them all fixed up.

@odeke-em odeke-em force-pushed the x-goog-spanner-request-id-plumb-into-BatchCreateSessions branch from cd1af14 to 8c48dec Compare May 16, 2025 21:11
@odeke-em odeke-em requested a review from olavloite May 16, 2025 21:46
@odeke-em odeke-em force-pushed the x-goog-spanner-request-id-plumb-into-BatchCreateSessions branch 2 times, most recently from 39fdba9 to 9484419 Compare May 20, 2025 01:46
@olavloite olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 20, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 20, 2025
@odeke-em odeke-em force-pushed the x-goog-spanner-request-id-plumb-into-BatchCreateSessions branch from 9484419 to d08e24c Compare May 22, 2025 03:22
odeke-em and others added 13 commits May 26, 2025 00:28
@odeke-em
More updates
c045751
@odeke-em
DatabaseClientImpl.runWithSessionRetry update on ID
f426d63
@odeke-em
Move reqId attempt increase out of runWithRetries
ee21a0c
@odeke-em
Apply allOptions copy everywhere
914b073
@odeke-em
Update tests for createMultiplexedSession
7c7218f
@odeke-em
chore(x-goog-spanner-request-id): plumb for BatchCreateSessions
96bb8f4 
This change plumbs x-goog-spanner-request-id into BatchCreateSessions
and asserts that the header is present for that method.

Updates googleapis#3537
@olavloite @odeke-em
test: fix failing test cases
f33a0d9
@odeke-em
Add setRequestIdCreator inside session tests
778cb0c
@odeke-em
Remove unnecessary debugs
3c10ee6
@odeke-em
Address review feedback
f76cbc0
@odeke-em
More code review updates
8c4f245
@odeke-em
More code review updates
127ea9a 
More updates

DatabaseClientImpl.runWithSessionRetry update on ID

Move reqId attempt increase out of runWithRetries

Apply allOptions copy everywhere

Complete testrunWithSessionRetry_withRequestId
@olavloite @odeke-em
chore: fix failing test cases
591ab72
@odeke-em odeke-em force-pushed the x-goog-spanner-request-id-plumb-into-BatchCreateSessions branch from 9608560 to 591ab72 Compare May 26, 2025 07:28
@olavloite olavloite added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels May 26, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 26, 2025
olavloite
olavloite approved these changes May 26, 2025
View reviewed changes
google-cloud-spanner/src/main/java/com/google/cloud/spanner/Options.java
Comment on lines +1090 to +1097
public int hashCode() {
return RequestIdOption.class.hashCode();
}

@Override
public boolean equals(Object o) {
return o instanceof RequestIdOption;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Regarding #3815 (comment)

LastStatementOption does not contain any state, and hence the hashCode and equals methods just returns the hash code of the class / checks if the other object is also an instance of the same class.

This option does contain state, and the hashCode and equals methods should use that state to calculate a hash value and check equality. Let's fix that in a follow-up PR.

@olavloite olavloite merged commit 10c3563 into googleapis:main May 26, 2025
44 of 54 checks passed
@odeke-em odeke-em deleted the x-goog-spanner-request-id-plumb-into-BatchCreateSessions branch May 26, 2025 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olavloite olavloite olavloite approved these changes

Assignees
No one assigned
Labels
api: spanner Issues related to the googleapis/java-spanner API. size: l Pull request size is large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@odeke-em @olavloite @rahul2393 @yoshi-kokoro