Skip to content

feat(bigtable): route read_row/mutate_row through the accelerator with native fallback - #18474

Merged
mutianf merged 5 commits into
googleapis:bigtable-accelfrom
mutianf:accel-02-squashed
Oct 2, 2026
Merged

mutianf merged 5 commits into
googleapis:bigtable-accelfrom
mutianf:accel-02-squashed

Conversation

@mutianf

@mutianf mutianf commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Routes read_row and mutate_row through the accelerator daemon with transparent native fallback
  • Implements a sticky fallback breaker: trips on UNIMPLEMENTED so a daemon that can't serve an RPC is bypassed without further round-trips
  • Adds _fallback.py, _routing.py, and _accelerator_client.py (async + sync) for the accelerator call path

Squashed from mutianf#2 (excluding daemon wrapper commits already in this branch).

Test plan

  • tests/unit/data/test_accelerator_enablement.py covers routing + fallback logic

@mutianf
mutianf requested a review from a team as a code owner September 25, 2026 19:39

@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 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).

Comment thread packages/google-cloud-bigtable/google/cloud/bigtable/data/_async/client.py Outdated
…h native fallback

Change-Id: I7aa01522d4d6de9aa790061b11b10465525bfb90
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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

how about authorized views and materialized views?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread packages/google-cloud-bigtable/google/cloud/bigtable/data/_async/client.py Outdated
Change-Id: I7b39c6dd44aa34ddbd84355f4c2c16389893918d
@mutianf

mutianf commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review

@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 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
@sushanb

sushanb commented Oct 2, 2026

Copy link
Copy Markdown

A few things worth addressing before this lands:

1. Operation metrics on the accelerator path — do we need _create_operation for CSM?

_read_row_via_accelerator / _mutate_row_via_accelerator build an ActiveOperationMetric (or skip it entirely for mutate_row) but never enter the self._create_operation(OperationType.X) context manager that the native path uses:

with self._create_operation(OperationType.READ_ROWS, is_streaming=False) as operation_metric:
    row_merger = CrossSync._ReadRowsOperation(..., metric=operation_metric, ...)
    ...

Since the accelerator is on by default, every routed read_row / mutate_row would stop appearing in client-side metrics once this lands — including the CSM per-attempt view (cluster_id/zone_id/latencies). Is that intentional (daemon-side telemetry will cover it), or do we need the same _create_operation wrapper on the accelerator path so CSM continues to see these ops? If the daemon is going to publish CSM, worth a comment pointing at that; if not, please wire the context manager through.

3. AcceleratorBreaker locking is half-there — please fix

def bypass(self) -> bool:
    return self._tripped          # unlocked read

def trip(self) -> None:
    with self._lock:
        self._tripped = True       # locked write

On CPython the GIL hides it, but on free-threaded 3.13+ this is a data race, and locking only the write gives bypass() no memory-ordering guarantee. Either drop the lock entirely (the transition is monotonic — a torn read at worst costs one extra dial), or model it as a threading.Event, which gives proper ordering and documents intent. The current "lock on write only" is the worst of both.

7. Platform gate for Windows — can you add it?

UDS is a non-starter on Windows, so with the accelerator defaulted on, every Table on Windows will spawn → fail → warn → degrade. Can you add a short-circuit at the top of _maybe_start_accelerator:

if sys.platform == "win32":
    if explicit:
        raise RuntimeError("use_accelerator=True is not supported on Windows")
    return

Avoids a subprocess spawn per Table on a platform where it can never work.

8. Shutdown race on _use_accelerator → RPC

Between _use_accelerator() returning True and self._accelerator_client.read_rows(...) dereferencing it, another thread (relevant in the generated sync client) can call close() which nils _accelerator_client. Result: AttributeError instead of a clean fallback or a translated error. Fix is a local snapshot inside _read_row_via_accelerator / _mutate_row_via_accelerator:

client = self._accelerator_client
if client is None:
    raise _AcceleratorFallback()
# use `client` for the rest of the method

Low probability per call, but a user who overlaps Table.close() with in-flight ops will see spurious errors that look like client bugs.

@mutianf

mutianf commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @sushanb!

1. Metrics on the accelerator path — intentional: the accelerator daemon publishes its own CSM telemetry (per-attempt cluster_id/zone_id/latencies), so the native _create_operation wrapper would double-count. The existing TODO(accelerator) comment in handle_accelerator_error is where we'll hook in a fallback/error counter once client-side accelerator metrics land; no change needed here.

3. AcceleratorBreaker locking — fixed. Replaced the bool + Lock (write-only lock, racy read) with threading.Event, which gives proper memory-ordering guarantees and documents the monotone-set semantics. The lock is gone entirely.

7. Windows platform gate — fixed. Added a sys.platform == 'win32' short-circuit at the top of _maybe_start_accelerator: raises RuntimeError when explicit, silently returns otherwise, so no subprocess is spawned on Windows.

8. Shutdown race — fixed. Both _read_row_via_accelerator and _mutate_row_via_accelerator now snapshot self._accelerator_client and self._accelerator_daemon into locals before use; a concurrent close() that nils the instance fields can't cause an AttributeError mid-flight.

…gate, shutdown race)

Change-Id: I13f44fa38a497023aae0392a36f1df9501bdf594
@mutianf
mutianf merged commit ed5878d into googleapis:bigtable-accel Oct 2, 2026
16 checks passed
@mutianf
mutianf deleted the accel-02-squashed branch October 2, 2026 21:19
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.

2 participants