Repository navigation
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 Windows support for the Bigtable accelerator daemon. It handles Windows-specific binary naming, adjusts socket liveness checks and stale directory sweeping to accommodate Windows filesystem and socket behaviors, and updates the gRPC socket target format to support drive-lettered paths. The review feedback points out a critical issue where checking hasattr(socket, 'AF_UNIX') is insufficient on Python 3.9+ because CPython exposes AF_UNIX on Windows, which would bypass the intended Windows fallback logic. It is recommended to use the _IS_WINDOWS constant directly instead.
d933d13 to
bdfb6c8
Compare
Change-Id: Icd8495d9c7f1bdcf39ae984b0037f813ef4c4011
…ilure Change-Id: Ie5c45b75e0785a284a0d241415cd5b8492e14e9c
… checks fix(bigtable): lock all reads and writes in AcceleratorBreaker Change-Id: I9d1daffbe086788481d6802cec035a3d4d284729
…, clear stale auth_secret on teardown Change-Id: I02c7ecb2d9ac5aa082bec0dc3325938b621bd210
Change-Id: I6665fec71ca24efe8ec47f95a71de6dda59abe5e
…), and breaker lock Change-Id: I21cf617dbea7aa85dc7b0d093e15c1ea22c7f95c
…degraded on exit Change-Id: I99ce4c0ffb7b12a3f1746801ddcadc4294f61b17
Change-Id: I7a17dd9919dff6e9d38b0cdfb3c600ffc363900c
… a raw socket connect Change-Id: I1f9899ed28dc504fbdbfdb2a93d34a404e187301
| # console control event to TerminateProcess, so that same call would | ||
| # *kill* the spawning Python process it is supposed to be asking | ||
| # about — someone else's live application. Porting it needs an | ||
| # OpenProcess/WaitForSingleObject check instead, which is left for a |
There was a problem hiding this comment.
The sweep is correctly disabled on Windows, but as noted the trade is that leftover tempdirs accumulate in %TEMP% across daemon restarts/crashes. Worth filing a tracked follow-up to port the liveness check to OpenProcess + GetExitCodeProcess (or WaitForSingleObject with timeout 0) so this doesn't quietly turn into a %TEMP% pollution bug report in six months.
| # tempfile.mkdtemp(prefix="bt-accel-") placement keeps the socket path well | ||
| # under the limit on every platform. | ||
| @pytest.fixture | ||
| def tmp_path(): |
There was a problem hiding this comment.
Module-scoped tmp_path override silently shadows pytest's built-in for every test in the file. Rationale (sockaddr_un.sun_path limit) is correct and well-commented, but a future contributor who adds a test here and expects pytest's standard tmp_path layout (e.g., for a fixture that assumes the pytest-of-user tree) will be confused. Suggest renaming to short_tmp_path or similar so tests opt in explicitly to the shortened path.
| monkeypatch.setattr(_daemon.os.path, "exists", lambda p: False) | ||
| # Nothing ever binds the socket, so the readiness channel stays in | ||
| # TRANSIENT_FAILURE and the deadline is what ends the wait. | ||
| monkeypatch.setenv(_STARTUP_TIMEOUT_ENV_VAR, "0.05") |
There was a problem hiding this comment.
This test used to monkeypatch os.path.exists to always return False so the UDS "could never appear"; after the readiness-probe switch it now relies on the gRPC channel staying in TRANSIENT_FAILURE for the 50 ms timeout. Fine today, but if the OS ever puts something at the real uds_path (unlikely with tempfile.mkdtemp's 0700 mode, but not impossible in odd CI setups), this test could spuriously pass the readiness probe and then fail an unrelated assertion. Consider re-adding an explicit "nothing is listening on this socket" guard — e.g., assert grpc.channel_ready_future(...).result(0.1) raises on a bare mkdtemp() path first, so the test anchors the failure mode.
sushanb
left a comment
sushanb
left a comment
There was a problem hiding this comment.
LGTM. Approach is sound — AF_UNIX end-to-end via gRPC C-core, channel_ready_future probe (a nice improvement on POSIX too, since bare connect() succeeded on the listen backlog before gRPC was accepting), and the pid-liveness sweep gated off until Windows gets an OpenProcess port. Three low-severity follow-ups above (Windows sweep tracking issue, tmp_path fixture naming, test_uds_never_appears guard). None are blockers.
The Go daemon gained Windows UDS support in googleapis/google-cloud-go#20455, so this lifts the
sys.platform == "win32"block and fixes the POSIX assumptions behind it.