Bundle log service + host manager in data_substrate; drop OPEN_LOG_SERVICE - #512
Merged
Conversation
…RVICE data_substrate now ships the log service, host manager, and tx-log-protos in-tree, so the separate clone/symlink build steps are no longer needed. - Bump data_substrate to the in-tree open-sourcing commit. - Remove the OPEN_LOG_SERVICE cmake option (WITH_LOG_SERVICE is kept). - Drop the eloq_log_service / raft_host_manager clone+symlink plumbing from GitHub Actions (build.yml), Concourse pipelines/tasks/scripts, and scripts/git-checkout.sh / git-tag.sh; the host_manager binary copy is kept. - Remove the obsolete log.pr.ent pipeline (it tested the now-absorbed eloq_log_service repo). - Remove stale -DOPEN_LOG_SERVICE / -DFORK_HM_PROCESS cmake flags. - Update CLAUDE.md build instructions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemoves ChangesExternal Dependency Removal and CMake Cleanup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Merged
4 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@concourse/scripts/build_tarball.bash`:
- Around line 205-207: Add defensive quoting around the CMake variable
expansions in the cmake configure command (lines 205-207). Specifically, wrap
the $ASAN and ${LOG_STATE_TYPE} variable expansions with double quotes so they
become "$ASAN" and "${LOG_STATE_TYPE}" respectively. This makes the intent
clearer and protects against future changes to variable sources, even though
these variables are currently validated to safe single-word values earlier in
the script.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d066ddb8-d368-408d-bc6c-8dac05bf17c9
📒 Files selected for processing (29)
.github/workflows/build.ymlCLAUDE.mdCMakeLists.txtconcourse/pipeline/build_debug_tarball.ymlconcourse/pipeline/build_nightly_tarball.ymlconcourse/pipeline/build_release_tarball.ymlconcourse/pipeline/build_tarball_open.ymlconcourse/pipeline/log.pr.ent.ymlconcourse/pipeline/main.ent.ymlconcourse/pipeline/pr.ent.ymlconcourse/pipeline/tag.ymlconcourse/scripts/build_tarball.bashconcourse/scripts/build_tarball_open.bashconcourse/scripts/common.shconcourse/scripts/log.pr.ent.bashconcourse/scripts/main.ent.bashconcourse/scripts/pr.ent.bashconcourse/scripts/tag.shconcourse/tasks/build_debug_tarball.ymlconcourse/tasks/build_nightly_tarball.ymlconcourse/tasks/build_release_tarball.ymlconcourse/tasks/build_tarball_open.ymlconcourse/tasks/log.pr.ent.ymlconcourse/tasks/main.ent.ymlconcourse/tasks/pr.ent.ymlconcourse/tasks/tag.ymldata_substratescripts/git-checkout.shscripts/git-tag.sh
💤 Files with no reviewable changes (25)
- concourse/pipeline/log.pr.ent.yml
- concourse/tasks/pr.ent.yml
- concourse/tasks/build_nightly_tarball.yml
- concourse/tasks/log.pr.ent.yml
- concourse/scripts/tag.sh
- concourse/tasks/build_tarball_open.yml
- concourse/tasks/main.ent.yml
- concourse/pipeline/main.ent.yml
- concourse/tasks/build_debug_tarball.yml
- concourse/scripts/log.pr.ent.bash
- concourse/tasks/build_release_tarball.yml
- concourse/tasks/tag.yml
- concourse/pipeline/build_release_tarball.yml
- concourse/scripts/main.ent.bash
- concourse/pipeline/build_tarball_open.yml
- .github/workflows/build.yml
- concourse/scripts/pr.ent.bash
- concourse/scripts/build_tarball_open.bash
- scripts/git-tag.sh
- scripts/git-checkout.sh
- concourse/pipeline/tag.yml
- concourse/pipeline/build_debug_tarball.yml
- concourse/pipeline/build_nightly_tarball.yml
- CMakeLists.txt
- concourse/pipeline/pr.ent.yml
Comment on lines
205
to
+207
| cmake .. -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DWITH_DATA_STORE=$DATA_STORE_TYPE $CMAKE_ARGS \ | ||
| -DWITH_LOG_SERVICE=ON -DDISABLE_CKPT_REPORT=${CKPT_REPORT_FLAG} -DDISABLE_CODE_LINE_IN_LOG=${CODE_LINE_FLAG} \ | ||
| -DWITH_ASAN=$ASAN -DOPEN_LOG_SERVICE=OFF -DFORK_HM_PROCESS=ON -DWITH_LOG_STATE=${LOG_STATE_TYPE} | ||
| -DWITH_ASAN=$ASAN -DWITH_LOG_STATE=${LOG_STATE_TYPE} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat concourse/scripts/build_tarball.bash | head -n 210 | tail -n 20Repository: eloqdata/eloqkv
Length of output: 794
🏁 Script executed:
# Get the context around lines 205-207
sed -n '195,215p' concourse/scripts/build_tarball.bashRepository: eloqdata/eloqkv
Length of output: 976
🏁 Script executed:
# Check if ASAN and LOG_STATE_TYPE variables are set elsewhere in the script
rg -n 'ASAN|LOG_STATE_TYPE' concourse/scripts/build_tarball.bashRepository: eloqdata/eloqkv
Length of output: 628
🏁 Script executed:
# Check for other cmake invocations in the script to compare quoting patterns
rg -n 'cmake' concourse/scripts/build_tarball.bash | head -20Repository: eloqdata/eloqkv
Length of output: 643
🏁 Script executed:
# Check how ASAN and LOG_STATE_TYPE are initialized/passed
sed -n '1,150p' concourse/scripts/build_tarball.bash | grep -E 'ASAN|LOG_STATE_TYPE|set -'Repository: eloqdata/eloqkv
Length of output: 426
🏁 Script executed:
# Check if these variables can be undefined
rg -B5 -A5 'ASAN=|LOG_STATE_TYPE=' concourse/scripts/build_tarball.bash | head -40Repository: eloqdata/eloqkv
Length of output: 1042
Quote CMake value expansions in the configure command.
Although ASAN and LOG_STATE_TYPE are validated to safe single-word values earlier in the script (lines 125-142), unquoted expansion at line 207 introduces unnecessary fragility. Defensive quoting makes the intent clearer and protects against future changes to variable sources.
Suggested fix
cmake .. -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DWITH_DATA_STORE=$DATA_STORE_TYPE $CMAKE_ARGS \
-DWITH_LOG_SERVICE=ON -DDISABLE_CKPT_REPORT=${CKPT_REPORT_FLAG} -DDISABLE_CODE_LINE_IN_LOG=${CODE_LINE_FLAG} \
- -DWITH_ASAN=$ASAN -DWITH_LOG_STATE=${LOG_STATE_TYPE}
+ "-DWITH_ASAN=${ASAN}" "-DWITH_LOG_STATE=${LOG_STATE_TYPE}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cmake .. -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DWITH_DATA_STORE=$DATA_STORE_TYPE $CMAKE_ARGS \ | |
| -DWITH_LOG_SERVICE=ON -DDISABLE_CKPT_REPORT=${CKPT_REPORT_FLAG} -DDISABLE_CODE_LINE_IN_LOG=${CODE_LINE_FLAG} \ | |
| -DWITH_ASAN=$ASAN -DOPEN_LOG_SERVICE=OFF -DFORK_HM_PROCESS=ON -DWITH_LOG_STATE=${LOG_STATE_TYPE} | |
| -DWITH_ASAN=$ASAN -DWITH_LOG_STATE=${LOG_STATE_TYPE} | |
| cmake .. -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DWITH_DATA_STORE=$DATA_STORE_TYPE $CMAKE_ARGS \ | |
| -DWITH_LOG_SERVICE=ON -DDISABLE_CKPT_REPORT=${CKPT_REPORT_FLAG} -DDISABLE_CODE_LINE_IN_LOG=${CODE_LINE_FLAG} \ | |
| "-DWITH_ASAN=${ASAN}" "-DWITH_LOG_STATE=${LOG_STATE_TYPE}" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 205-205: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 205-205: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 205-205: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 207-207: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 207-207: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@concourse/scripts/build_tarball.bash` around lines 205 - 207, Add defensive
quoting around the CMake variable expansions in the cmake configure command
(lines 205-207). Specifically, wrap the $ASAN and ${LOG_STATE_TYPE} variable
expansions with double quotes so they become "$ASAN" and "${LOG_STATE_TYPE}"
respectively. This makes the intent clearer and protects against future changes
to variable sources, even though these variables are currently validated to safe
single-word values earlier in the script.
Source: Linters/SAST tools
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ervice-hostmanager
thweetkomputer
approved these changes
Jun 16, 2026
liunyl
added a commit
to eloqdata/tx_service
that referenced
this pull request
Jun 16, 2026
#509) Fold the log service, host manager, and tx-log-protos directly into data_substrate as in-tree source under the repository's root Business Source License 2.0 (the same model as eloqstore), and remove the build toggles that distinguished the open vs proprietary variants. - Absorb eloq_log_service/ and tx_service/raft_host_manager/ (were gitignored nested clones) and tx_service/tx-log-protos/ (was a submodule) as tracked in-tree files; drop tx-log-protos's GPL/AGPL LICENSE (now covered by the root BSL). - Remove the open-source log_service submodule and build_log_service.cmake. - Remove the OPEN_LOG_SERVICE option; always build the in-tree eloq_log_service via build_eloq_log_service.cmake (target logservice) under WITH_LOG_SERVICE. - Remove the FORK_HM_PROCESS option; the host manager is always built, installed, and forked. - Collapse the now-dead OPEN_LOG_SERVICE / FORK_HM_PROCESS preprocessor guards, keeping the eloq / forked code paths. - Consolidate the duplicate vendored inih/INIReader copies into third_party/ini. - Add a log-service CI workflow that builds the RocksDB and RocksDB-Cloud(S3) variants and runs the cloud unit tests against MinIO. - Refresh module design docs (01, 06, 07, 08, 10, README) and CLAUDE.md. WITH_LOG_SERVICE is unchanged. Paired with eloqdata/eloqkv#512, which bumps this submodule and removes the matching CI clone/symlink plumbing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Re-point the submodule from the PR-branch commit to the squash-merged tx_service main (#509); identical tree, so CI validation is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
liunyl
added a commit
that referenced
this pull request
Jun 29, 2026
Squash of the 10 chore/data-substrate-third-party-workspace commits, rebased onto current main and reconciled with main's #512 bundling: - Third-party workspace wiring: build/docker/release CI, dependency scripts (scripts/dep, install_dependency_ubuntu2404.sh) and scripts/checkout_product_submodules.sh. - Re-pin data_substrate to the reconciled third-party-workspace commit. - log_service and host_manager are bundled in-tree on main (#509/#512); drop the CI step that cloned the private repos and the dead OPEN_LOG_SERVICE / FORK_HM_PROCESS flags. No source changes are needed: the workspace abseil now uses its default options (ABSL_OPTION_USE_*=2), matching main's abseil submodule. Build verification pending. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Companion to eloqdata/tx_service#509.
data_substratenow bundles the log service, host manager, and tx-log-protos in-tree, so the separate clone/symlink build steps are no longer needed.data_substratesubmodule to the in-tree open-sourcing commit (Open-source log service, host manager, and tx-log-protos in-tree (BSL) tx_service#509).OPEN_LOG_SERVICEcmake option (WITH_LOG_SERVICEis kept).build.yml), Concourse pipelines/tasks/scripts, andscripts/git-checkout.sh/git-tag.sh; thehost_managerbinary copy steps are kept.log.pr.entConcourse pipeline (it tested the now-absorbedeloq_log_servicerepo).-DOPEN_LOG_SERVICE/-DFORK_HM_PROCESScmake flags.CLAUDE.mdbuild instructions.Test plan
cmakeconfigure +cmake --build(Debug) succeeds; produceseloqkvanddata_substrate/host_manager.log.pr.entremoval has no remaining references.🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Documentation
WITH_LOG_SERVICEinstead ofOPEN_LOG_SERVICE, including example CMake arguments.Refactor
OPEN_LOG_SERVICEbuild option and updated build-flag forwarding to match the new configuration.