Skip to content

test(bigtable): add accelerator read/write correctness and concurrency tests - #18788

Open
mutianf wants to merge 1 commit into
bigtable-accelfrom
accel13
Open

mutianf wants to merge 1 commit into
bigtable-accelfrom
accel13

Conversation

@mutianf

@mutianf mutianf commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Add differential read/write correctness system tests (test_fuzz_correctness_async.py, test_fuzz_correctness_autogen.py) and concurrent/multi-process workload tests (test_concurrency_async.py, test_concurrency_autogen.py).

Stacked on #18787.

@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 Bigtable accelerator daemon lifecycle, including both asynchronous (test_lifecycle_async.py) and auto-generated synchronous (test_lifecycle_autogen.py) test suites. The tests verify daemon reaping, resource reclamation after crashes, and leak prevention. Feedback on the changes highlights that the asynchronous test file uses synchronous time.sleep() calls inside async def methods, which blocks the event loop. It is recommended to import asyncio and replace these with non-blocking await asyncio.sleep() calls.

Comment on lines +15 to +20
import multiprocessing
import os
import shutil
import signal
import time
import uuid

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.

medium

Import asyncio to enable non-blocking sleeps in asynchronous test methods.

Suggested change
import multiprocessing
import os
import shutil
import signal
import time
import uuid
import asyncio
import multiprocessing
import os
import shutil
import signal
import time
import uuid

os.kill(pid, signal.SIGKILL)
deadline = time.monotonic() + 10.0
while _harness.pid_alive(pid) and time.monotonic() < deadline:
time.sleep(0.05)

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.

medium

Using synchronous time.sleep() inside an async def test method blocks the entire event loop. Use await asyncio.sleep() instead to keep the event loop responsive.

Suggested change
time.sleep(0.05)
await asyncio.sleep(0.05)

assert not owner.is_alive(), "owner process not reaped after SIGKILL"
deadline = time.monotonic() + 30.0
while _harness.pid_alive(daemon_pid) and time.monotonic() < deadline:
time.sleep(0.1)

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.

medium

Using synchronous time.sleep() inside an async def test method blocks the entire event loop. Use await asyncio.sleep() instead to keep the event loop responsive.

Suggested change
time.sleep(0.1)
await asyncio.sleep(0.1)

@mutianf mutianf changed the title test(bigtable): add accelerator daemon lifecycle system tests test(bigtable): add accelerator read/write correctness and concurrency tests Nov 13, 2026
@mutianf
mutianf force-pushed the accel13 branch 3 times, most recently from 06d4e91 to 9f9b911 Compare November 13, 2026 19:34
Base automatically changed from accel12 to bigtable-accel November 13, 2026 19:44

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