Skip to content

add gflag fork_from_txservice#343

Merged
lokax merged 1 commit into
mainfrom
yf-fix-hm-attach
Jan 7, 2026
Merged

add gflag fork_from_txservice#343
lokax merged 1 commit into
mainfrom
yf-fix-hm-attach

Conversation

@lokax

@lokax lokax commented Jan 7, 2026

Copy link
Copy Markdown
Collaborator

Here are some reminders before you submit the pull request

  • Add tests for the change
  • Document changes
  • Reference the link of issue using fixes eloqdb/tx_service#issue_id
  • Reference the link of RFC if exists
  • Pass ./mtr --suite=mono_main,mono_multi,mono_basic

Summary by CodeRabbit

  • Chores
    • Enhanced internal process management infrastructure to improve system reliability and inter-component coordination
    • Refined system-level initialization mechanisms for better resource handling and stability during service startup

✏️ Tip: You can customize this high-level summary in your review settings.

Copilot AI review requested due to automatic review settings January 7, 2026 07:27
@coderabbitai

coderabbitai Bot commented Jan 7, 2026

Copy link
Copy Markdown

Walkthrough

The change adds a new command-line flag --fork_from_txservice=true to the host manager launch path when FORK_HM_PROCESS is enabled. The flag is constructed in initialization and passed as an argument during process spawning, establishing a signaling mechanism from tx_service to its forked host manager process.

Changes

Cohort / File(s) Summary
Host manager fork signaling
tx_service/src/sharder.cpp
Adds --fork_from_txservice=true flag construction and passes it to the forked host manager process via posix_spawn argument vector when FORK_HM_PROCESS is enabled.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • fix fork host manager gflag #308: Fixes handling of the existing fork_host_manager gflag during initialization; related to the same host manager forking behavior.

Suggested reviewers

  • liunyl
  • lzxddz

Poem

🐰 A flag takes flight on forked wings so bright,
From parent to child in the spawning night,
"Fork from tx_service!" the signal declares,
A whispered handshake through process affairs!

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description contains only the template checklist with all items unchecked, providing no substantive information about the PR's purpose, implementation, or rationale. Fill in the checklist items and provide a meaningful description explaining the purpose of the fork_from_txservice flag, how it's used, and why it's needed.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title 'add gflag fork_from_txservice' directly describes the main change—adding a new command-line flag to the codebase, which aligns with the file-level summary of introducing the '--fork_from_txservice' flag.

✏️ Tip: You can configure your own custom Pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch yf-fix-hm-attach

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 876056c and 587eca3.

📒 Files selected for processing (1)
  • tx_service/src/sharder.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Agent
🔇 Additional comments (1)
tx_service/src/sharder.cpp (1)

339-339: Verify that the host_manager binary recognizes the --fork_from_txservice=true flag.

The new flag being passed to the host_manager process at lines 339 and 349 cannot be found defined in the codebase (no DEFINE statement for it), which suggests either the host_manager source is unavailable in this repository or the flag may not be recognized. Confirm that the host_manager build includes argument parsing for this flag.

Also applies to: 349-349


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a new gflag fork_from_txservice that is passed to the host_manager process when it's spawned from the tx_service. The flag is set to true to indicate that the host_manager was forked from the tx_service process.

Key changes:

  • Added --fork_from_txservice=true flag to the command-line arguments passed to the host_manager process during forking

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

std::string arg_raft_log = "--hm_raft_log_path=" + log_path;
std::string arg_cluster_config_path =
"--cluster_config_path=" + cluster_config_path;
std::string fork_from_txservice = "--fork_from_txservice=true";

Copilot AI Jan 7, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flag fork_from_txservice is being passed to the host_manager process, but there's no visible definition of this flag (DEFINE_bool) in the codebase. This could cause the host_manager process to fail if it doesn't recognize this flag. Ensure that the corresponding flag definition exists in the host_manager code, or document where this flag is expected to be defined.

Copilot uses AI. Check for mistakes.
@@ -336,6 +336,7 @@ int Sharder::Init(
std::string arg_raft_log = "--hm_raft_log_path=" + log_path;
std::string arg_cluster_config_path =
"--cluster_config_path=" + cluster_config_path;

Copilot AI Jan 7, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new command-line argument is being added to the host_manager process without any documentation explaining its purpose. Add a comment explaining what this flag is for and why it's needed when the host_manager is forked from tx_service.

Suggested change
"--cluster_config_path=" + cluster_config_path;
"--cluster_config_path=" + cluster_config_path;
// Inform host_manager that it is being started as a child of tx_service
// rather than as a standalone binary. This allows host_manager to adjust
// its initialization/bootstrapping logic (for example, using the cluster
// configuration and Raft log path provided by tx_service) when it is
// forked from tx_service.

Copilot uses AI. Check for mistakes.
@lokax lokax merged commit 646d45d into main Jan 7, 2026
10 checks passed
@lokax lokax deleted the yf-fix-hm-attach branch February 26, 2026 10:14
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.

3 participants