Repository navigation
Conversation
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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:
pytest.warns(RuntimeWarning)will fail because no warning is emitted duringclient.get_table.elapsedis measured before the connection attempt actually happens, meaningelapsedwill be near 0 and the assertionelapsed >= delay * 0.5will 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.
| 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 |
| 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 |
There was a problem hiding this comment.
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:
pytest.warns(RuntimeWarning)will fail because no warning is emitted duringclient.get_table.elapsedis measured before the connection attempt actually happens, meaningelapsedwill be near 0 and the assertionelapsed >= delay * 0.5will 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.
| 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 |
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).