Skip to content

test(bigtable): add accelerator fallback, config, metrics, and startup parity tests - #18789

Open
mutianf wants to merge 1 commit into
accel13from
accel14
Open

mutianf wants to merge 1 commit into
accel13from
accel14

Conversation

@mutianf

@mutianf mutianf commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Add Cloud Bigtable accelerator fallback, configuration forwarding, client-side OpenTelemetry metrics parity, and identity-mismatch startup system tests (test_default_and_fallback_*, test_config_forwarding_*, test_metrics_parity_*, test_startup_*).

Part of Stack #18691 (stacked on #18788).

@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 system tests for the Google Cloud Bigtable Accelerator, covering configuration forwarding, default/fallback behavior, metrics parity, and startup fallback scenarios. The review feedback identifies a critical issue in the startup tests (both async and autogenerated sync versions), where the daemon startup and connection attempts are deferred until entering the table's context manager. Measuring elapsed time and wrapping pytest.warns around client.get_table instead of the context manager block will cause the assertions to fail. The feedback provides code suggestions to correctly place these measurements inside the table context manager block.

Comment on lines +43 to +53
started = time.monotonic()
async with self._make_client() as client:
with pytest.warns(RuntimeWarning):
table = client.get_table(instance_id, table_id)
elapsed = time.monotonic() - started
async with table:
assert elapsed >= delay * 0.5, (
f"start returned in {elapsed:.2f}s, before the {delay}s bind; "
"it did not wait for the UDS to become connectable"
)
assert table._accelerator_client is None

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.

high

The client.get_table method is a synchronous factory method that only instantiates the Table object; it does not trigger the daemon startup or connection attempt. These actions are deferred until the table's context manager is entered (i.e., during async with table:).

As a result:

  1. pytest.warns(RuntimeWarning) will fail because no warning is emitted during client.get_table.
  2. elapsed is measured before the connection attempt actually happens, meaning elapsed will be near 0 and the assertion elapsed >= delay * 0.5 will fail.

Moving the timing measurement and the pytest.warns block inside the async with table: block ensures the test correctly measures the startup delay and captures the warning.

Suggested change
started = time.monotonic()
async with self._make_client() as client:
with pytest.warns(RuntimeWarning):
table = client.get_table(instance_id, table_id)
elapsed = time.monotonic() - started
async with table:
assert elapsed >= delay * 0.5, (
f"start returned in {elapsed:.2f}s, before the {delay}s bind; "
"it did not wait for the UDS to become connectable"
)
assert table._accelerator_client is None
async with self._make_client() as client:
table = client.get_table(instance_id, table_id)
started = time.monotonic()
with pytest.warns(RuntimeWarning):
async with table:
elapsed = time.monotonic() - started
assert elapsed >= delay * 0.5, (
f"start returned in {elapsed:.2f}s, before the {delay}s bind; "
"it did not wait for the UDS to become connectable"
)
assert table._accelerator_client is None

Comment on lines +35 to +44
started = time.monotonic()
with self._make_client() as client:
with pytest.warns(RuntimeWarning):
table = client.get_table(instance_id, table_id)
elapsed = time.monotonic() - started
with table:
assert elapsed >= delay * 0.5, (
f"start returned in {elapsed:.2f}s, before the {delay}s bind; it did not wait for the UDS to become connectable"
)
assert table._accelerator_client is None

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.

high

The client.get_table method is a synchronous factory method that only instantiates the Table object; it does not trigger the daemon startup or connection attempt. These actions are deferred until the table's context manager is entered (i.e., during with table:).

As a result:

  1. pytest.warns(RuntimeWarning) will fail because no warning is emitted during client.get_table.
  2. elapsed is measured before the connection attempt actually happens, meaning elapsed will be near 0 and the assertion elapsed >= delay * 0.5 will fail.

Moving the timing measurement and the pytest.warns block inside the with table: block ensures the test correctly measures the startup delay and captures the warning.

Suggested change
started = time.monotonic()
with self._make_client() as client:
with pytest.warns(RuntimeWarning):
table = client.get_table(instance_id, table_id)
elapsed = time.monotonic() - started
with table:
assert elapsed >= delay * 0.5, (
f"start returned in {elapsed:.2f}s, before the {delay}s bind; it did not wait for the UDS to become connectable"
)
assert table._accelerator_client is None
with self._make_client() as client:
table = client.get_table(instance_id, table_id)
started = time.monotonic()
with pytest.warns(RuntimeWarning):
with table:
elapsed = time.monotonic() - started
assert elapsed >= delay * 0.5, (
f"start returned in {elapsed:.2f}s, before the {delay}s bind; it did not wait for the UDS to become connectable"
)
assert table._accelerator_client is None

This branch has not been deployed

No deployments
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.

1 participant