Skip to content

feat: add accelerator daemon process wrapper - #18383

Open
mutianf wants to merge 2 commits into
googleapis:bigtable-accelfrom
mutianf:accel-01-daemon-subprocess-wrapper
Open

mutianf wants to merge 2 commits into
googleapis:bigtable-accelfrom
mutianf:accel-01-daemon-subprocess-wrapper

Conversation

@mutianf

@mutianf mutianf commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

@mutianf
mutianf requested a review from a team as a code owner September 15, 2026 20:19

@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 the AcceleratorDaemon class to manage the lifecycle of the Go accelerator daemon subprocess, including spawning the binary, waiting for its Unix domain socket (UDS) to become connectable, and handling teardown. The review feedback identifies several critical robustness and cross-platform compatibility issues: a potential file-locking issue on Windows when cleaning up after a failed spawn, platform-specific errors due to the direct use of socket.AF_UNIX and signal.SIGKILL (which are not supported on all platforms), and a missing cleanup step for self._log_path in _cleanup_tempdir.

Change-Id: I576365629b0f737edc86815208351759d548b734
@mutianf
mutianf force-pushed the accel-01-daemon-subprocess-wrapper branch from da7a290 to 26ed3b9 Compare September 15, 2026 20:31
@mutianf mutianf changed the title Accel 01 daemon subprocess wrapper feat: add accelerator daemon process wrapper Sep 15, 2026
Change-Id: I7c218be39c2f62a87e8ca8e924717d6091d97ed7
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