Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an in-process accelerator daemon routing layer for the Bigtable Data API, enabling transparent fallback to the native client if the daemon is unavailable, degraded, or fails. It adds async and sync accelerator clients, routing logic, and fallback mechanisms. Feedback on the changes highlights a critical bug where an async call to read_rows is not awaited, as well as opportunities to improve robustness and cross-platform portability by restoring defensive exception handling during process teardown and using proc.kill() instead of send_signal(signal.SIGKILL).
…h native fallback Change-Id: I7aa01522d4d6de9aa790061b11b10465525bfb90
ed424fc to
38e4d0a
Compare
| class _AcceleratorFallback(Exception): | ||
| """Internal signal that an accelerator attempt should be retried natively. | ||
|
|
||
| Never escapes the Table method that raises it: the method catches it and |
There was a problem hiding this comment.
how about authorized views and materialized views?
There was a problem hiding this comment.
Good catch. The daemon only supports plain table routing (project/instance/app-profile), so accelerator calls from AuthorizedView or MaterializedView targets would always fall back after the first UNIMPLEMENTED. Fixed in the latest commit: _maybe_start_accelerator now returns early when authorized_view_id or materialized_view_id is set, so no daemon is spawned for those targets. Also updated the _AcceleratorFallback and AcceleratorBreaker docstrings to say "data target" instead of "Table".
There was a problem hiding this comment.
Correction to my previous reply: the daemon does support AuthorizedView and MaterializedView targets, so no bypass is needed. I've reverted that change. The only update in the latest commit is the docstring: "Table" → "data target" throughout _fallback.py and client.py.
Change-Id: I7b39c6dd44aa34ddbd84355f4c2c16389893918d
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces client-side fallback policy and routing for accelerator-routed RPCs in Google Cloud Bigtable, including daemon lifecycle management, health tracking, and fallback logic for read_row and mutate_row operations. Feedback on the changes suggests improving the robustness of the cleanup process in the close method of the client by wrapping individual resource closing calls in try-except blocks to ensure that an exception in closing the accelerator client does not prevent the accelerator daemon from being closed.
- close() now logs warnings instead of silently leaking the daemon when the client close raises; each resource is still nulled out via finally - _maybe_start_accelerator skips for AuthorizedView/MaterializedView targets (the daemon only supports plain table routing) - AcceleratorBreaker docstring updated to reflect all data target types Change-Id: I70148bbcde633e7327ff20f6e90543fc3bf079be
Daemon supports all data target types; the bypass was incorrect. Only simplify AcceleratorBreaker docstring to say "data target". Change-Id: I4687476c908f6c65574b54ef2ce87c284f224025
This branch has not been deployed
1 participant
Summary
read_rowandmutate_rowthrough the accelerator daemon with transparent native fallbackUNIMPLEMENTEDso a daemon that can't serve an RPC is bypassed without further round-trips_fallback.py,_routing.py, and_accelerator_client.py(async + sync) for the accelerator call pathSquashed from mutianf#2 (excluding daemon wrapper commits already in this branch).
Test plan
tests/unit/data/test_accelerator_enablement.pycovers routing + fallback logic