Skip to content

fix(bigtable): report swallowed batch flush errors and unacknowledged entries - #18122

Open
mutianf wants to merge 3 commits into
googleapis:mainfrom
mutianf:bigtable-batcher-partial-failure-dataloss
Open

fix(bigtable): report swallowed batch flush errors and unacknowledged entries#18122
mutianf wants to merge 3 commits into
googleapis:mainfrom
mutianf:bigtable-batcher-partial-failure-dataloss

Conversation

@mutianf

@mutianf mutianf commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

report swallowed batch flush errors and unacknowledged mutate_rows entries.

Double check if there's any active requests left to avoid incomplete mutations.

… mutate_rows entries

Change-Id: I61a1444656d46f5b8e62aea0d16670dc9115659f
@mutianf
mutianf requested a review from a team as a code owner August 14, 2026 17:52

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request improves error handling in Google Cloud Bigtable by surfacing exceptions raised during asynchronous flushes in the batcher and ensuring that mutations without response entries in successfully closed streams are treated as incomplete rather than silently dropped. The review feedback points out a potential CancelledError when calling future.exception() on a cancelled future in _batch_completed_callback, suggesting a check for future.cancelled() to avoid unhandled exceptions.

Comment thread packages/google-cloud-bigtable/google/cloud/bigtable/batcher.py
Change-Id: Ib6727718ec0e39dd7aad1298532b5e2e64439070

@daniel-sanche daniel-sanche left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@mutianf
mutianf enabled auto-merge (squash) August 14, 2026 18:20
@parthea parthea added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Aug 14, 2026
@yoshi-kokoro yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Aug 14, 2026
@daniel-sanche daniel-sanche changed the title fix(bigtable): report swallowed batch flush errors and unacknowledged… fix(bigtable): report swallowed batch flush errors and unacknowledged entries Aug 14, 2026

@parthea parthea left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The unit tests seem to be running for more than an hour. Please can you check if it is related to the changes in this PR?

https://github.com/googleapis/google-cloud-python/actions/runs/31826948380/attempts/1?pr=18122

image

@parthea
parthea disabled auto-merge August 14, 2026 19:10
@parthea parthea added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 14, 2026
@parthea

parthea commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Adding do not merge to investigate the delay with the unit tests. In the last PR that was merged to packages/google-cloud-bigtable, tests only took 7 minutes

#18112

https://github.com/googleapis/google-cloud-python/actions/runs/31780116767

image

@parthea parthea left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Something is wrong with tests. The duration of unit tests increased from ~ 7 minutes to > 1 hour

@daniel-sanche

Copy link
Copy Markdown
Contributor

Hmm I wonder if the fork could be causing the issue? I'm looking into this

@mutianf

mutianf commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

The errors seems to be:

 raise exceptions.from_grpc_error(exc) from exc
E           google.api_core.exceptions.ResourceExhausted: 429 Quota exceeded for quota metric 'Instance and cluster metadata write requests' and limit 'Instance and cluster metadata write requests per day' of service 'bigtableadmin.googleapis.com' for consumer 'project_number:'. [reason: "RATE_LIMIT_EXCEEDED"

@mutianf

mutianf commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

I think my fix on the async client broke the test:

The async MutateRows operation retries by tracking remaining_indices — the set of entries still needing a response. At the start of each attempt it resets remaining_indices = [] and only re-adds an entry when the server explicitly returns a non-OK status for it. This means that if the stream closes cleanly (gRPC OK) but simply omits the response entry for a mutation, that entry is neither re-queued nor recorded as an error — it's silently treated as successful. Since the server sends one response entry per request entry and the client cannot know the true outcome of an omitted one, assuming success risks reporting a write as applied when it may not have been (silent data loss). The correct behavior is to treat an omitted entry as an unknown outcome: retry it if idempotent, or surface it as a failure otherwise.

The catch is that the existing V3 unit tests bake in the old assumption — their mock responses return an entry only for the mutations they care about and let the rest be "omitted = success" (e.g. _mock_response([DeadlineExceeded]) returns a single entry for a 3-entry request). So fixing the omitted-entry handling changes established behavior and breaks
those tests, which is what surfaced in CI.

… batcher fix

Change-Id: If379b98672c53286ab3dcb3d788006fbeb027b9c
@mutianf mutianf removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 14, 2026
@daniel-sanche
daniel-sanche requested a review from parthea August 14, 2026 21:06
@parthea
parthea enabled auto-merge (squash) August 14, 2026 22:03
@daniel-sanche daniel-sanche added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 15, 2026
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants