chore(x-goog-spanner-request-id): more updates for batch_write + mockserver tests #1375
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
product-auto-label
bot
added
size: m
|
Kindly help me run the bots on this @olavloite @surbhigarg92 @harshachinta @sakthivelmanii |
odeke-em
force-pushed
the
x-goog-request-id-continuation-2-of-3
branch
3 times, most recently
from
19e341a to
2cd1502
Compare
product-auto-label
bot
added
size: l
odeke-em
force-pushed
the
x-goog-request-id-continuation-2-of-3
branch
3 times, most recently
from
8000c2c to
64bd582
Compare
odeke-em
changed the title
odeke-em
force-pushed
the
x-goog-request-id-continuation-2-of-3
branch
from
64bd582 to
fa77dd1
Compare
olavloite
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
|
Kindly help me re-run those bots @olavloite |
olavloite
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
odeke-em
force-pushed
the
x-goog-request-id-continuation-2-of-3
branch
from
3af40e4 to
4cd0c31
Compare
olavloite
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
olavloite
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
olavloite
reviewed
May 19, 2025
|
@olavloite @surbhigarg92 @sakthivelmanii @harshachinta @rahul2393 kindly help me re-run the bots. Thank you. |
odeke-em
force-pushed
the
x-goog-request-id-continuation-2-of-3
branch
from
4ac9fef to
422f858
Compare
olavloite
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
olavloite
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
olavloite
reviewed
May 20, 2025
google/cloud/spanner_v1/pool.py
Outdated
Comment on lines
279
to
285
| allowed_exceptions={ | ||
| InternalServerError: _check_rst_stream_error, | ||
| ServiceUnavailable: _check_unavailable, | ||
| }, |
There was a problem hiding this comment.
We should not add new retry logic here (and in general also not elsewhere). The general rules for retries are:
- For unary RPCs: We rely on Gax handling all retries. This means that any RPC that has been configured as retryable at that level are automatically retried by Gax. We don't need to do anything in the handwritten client library. Currently, the retry configuration specifies that
UNAVAILABLEandRESOUCE_EXHAUSTEDare retryable error codes. But the client does not need to know, as this is handled by Gax. - For streaming RPCs: The have disabled retries by Gax by not adding any retryable error codes to the configuration. Retries of streaming RPCs therefore need to be handled manually in the handwritten client. Streaming RPCs should be retried in case of an UNAVAILABLE error, OR if it is one of the specific INTERNAL errors. (Note however that it is fair to consider the current retry implementation as complete, even if for example specific INTERNAL errors are currently not retried. Adding support for request-id is not about fixing potentially missing retry logic.)
- For transactions, we retry the entire transaction if the transaction is aborted by Spanner. This type of retry is 'irrelevant' for the request-id. With 'irrelevant' I mean that these retries do not need any special handling, and should not lead to the attempt number being increased.
olavloite
reviewed
May 20, 2025
|
|
||
| def test_unary_retryable_error(self): | ||
| add_select1_result() | ||
| add_error(SpannerServicer.BatchCreateSessions.__name__, unavailable_status()) |
There was a problem hiding this comment.
I changed the error code here from INTERNAL to UNAVAILABLE, because:
INTERNALwas not retried before for unary RPCs, and this change should not add retries for that. The specificINTERNALerrors that are retried are only relevant for streaming RPCs.- This PR seems to try to add additional retry handling for
UNAVAILABLEerrors. That handling is however never used, because the RPC itself is already retried by Gax, and this happens at a lower level. This again causes this test to seem to work, however the attempt number is not being increased during these retries. Because this test initially usedINTERNALto test, the additional retry handler was being invoked, and that did increase the attempt number.
…server tests This change plumbs in some x-goog-spanner-request-id updates for batch_write and some tests too. Updates googleapis#1261
nox -s blacken to format
odeke-em
force-pushed
the
x-goog-request-id-continuation-2-of-3
branch
from
c62dab6 to
092efe5
Compare
odeke-em
force-pushed
the
x-goog-request-id-continuation-2-of-3
branch
from
3da8c25 to
67d0131
Compare
|
@olavloite I've added a TODO so that we can firstly merge the code in and then can fully focus on figuring out request-id with unary retries. Kindly please take a look at the existing code |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change plumbs in some x-goog-spanner-request-id updates for batch_write and some tests too.
Updates #1261