Conversation
WalkthroughRefactors build scripts and Concourse pipelines: reworks workspace and git submodule orchestration, changes CMake to Ninja with shared libs, adjusts data-store/DSS paths and conditional eloqstore wiring, updates binary rpath handling and artifact task names in CI pipelines. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (6)📓 Common learnings📚 Learning: 2025-09-25T11:58:50.446ZApplied to files:
📚 Learning: 2025-09-25T12:24:06.434ZApplied to files:
📚 Learning: 2025-11-28T10:52:25.527ZApplied to files:
📚 Learning: 2025-11-28T10:55:21.020ZApplied to files:
📚 Learning: 2025-09-25T12:09:01.276ZApplied to files:
🔇 Additional comments (7)
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
concourse/scripts/build_tarball.bash (2)
79-82: Missing safe.directory entry for eloqstore_src on RHEL 9 causes build failure with ELOQDSS_ELOQSTORE.When
DATA_STORE_TYPE=ELOQDSS_ELOQSTOREon RHEL 9, the script symlinkseloqstore_src(line 41) and then attemptsgit submodule syncon it (line 43). However,eloqstore_srcis not in the safe.directory list, causing git to reject the operation with a "dubious ownership" error.Add
eloqstore_srcto safe.directory conditionally within the RHEL 9 block:git config --global --add safe.directory ${WORKSPACE}/eloqdoc_src git config --global --add safe.directory ${WORKSPACE}/eloq_logservice_src git config --global --add safe.directory ${WORKSPACE}/raft_host_manager_src + if [ "${DATA_STORE_TYPE}" = "ELOQDSS_ELOQSTORE" ]; then + git config --global --add safe.directory ${WORKSPACE}/eloqstore_src + fi
126-126: Update error messages to include ELOQDSS_ELOQSTORE as supported DATA_STORE_TYPE.Both error messages omit
ELOQDSS_ELOQSTOREfrom the supported types list, even though it is validated and handled at lines 137-138. This inconsistency can mislead developers.Update line 126:
- echo "DATA_STORE_TYPE must be provided. Supported: ELOQDSS_ROCKSDB_CLOUD_S3, ELOQDSS_ROCKSDB_CLOUD_GCS, ELOQDSS_ROCKSDB" + echo "DATA_STORE_TYPE must be provided. Supported: ELOQDSS_ROCKSDB_CLOUD_S3, ELOQDSS_ROCKSDB_CLOUD_GCS, ELOQDSS_ROCKSDB, ELOQDSS_ELOQSTORE"Update line 140:
- echo "Unsupported DATA_STORE_TYPE: ${DATA_STORE_TYPE}. Supported: ELOQDSS_ROCKSDB_CLOUD_S3, ELOQDSS_ROCKSDB_CLOUD_GCS, ELOQDSS_ROCKSDB" + echo "Unsupported DATA_STORE_TYPE: ${DATA_STORE_TYPE}. Supported: ELOQDSS_ROCKSDB_CLOUD_S3, ELOQDSS_ROCKSDB_CLOUD_GCS, ELOQDSS_ROCKSDB, ELOQDSS_ELOQSTORE"Also applies to: 140-140
🧹 Nitpick comments (2)
concourse/scripts/build_tarball.bash (2)
40-45: Inconsistent pushd/popd pattern compared to eloq_log_service.Lines 30-33 correctly use
pushdto enter eloq_log_service. Lines 40-45 use a different pattern:pushdto store_handler path, thencd eloqstore, thenpopd. For consistency and clarity, use nestedpushd/popd:pushd src/mongo/db/modules/eloq/data_substrate/store_handler/eloq_data_store_service ln -s $WORKSPACE/eloqstore_src eloqstore - cd eloqstore + pushd eloqstore git submodule sync git submodule update --init --recursive + popd popd
294-298: Clarify DSS_CMAKE_ARGS initialization.Line 295 appends to
${DSS_CMAKE_ARGS}which is undefined at this point. While bash word splitting handles this gracefully (expands to empty string), explicit initialization improves clarity for future maintainers:+ DSS_CMAKE_ARGS="" if [ "${DATA_STORE_TYPE}" = "ELOQDSS_ELOQSTORE" ]; then DSS_CMAKE_ARGS="${DSS_CMAKE_ARGS} -DELOQ_MODULE_ENABLED=ON" fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
concourse/scripts/build_tarball.bash(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: concourse/scripts/main.ent.bash:38-39
Timestamp: 2025-11-28T10:52:25.527Z
Learning: In the eloqdoc enterprise build (main.ent.bash), eloq_log_service is fetched separately as the resource `eloq_logservice_src` in the pipeline (main.ent.yml) and then symlinked into the data_substrate directory structure at build time using `ln -s $WORKSPACE/eloq_logservice_src eloq_log_service`. This is the intended architecture for placing eloq_log_service within data_substrate in the enterprise configuration.
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_log_service.cmake:116-119
Timestamp: 2025-09-25T11:58:50.446Z
Learning: The build_log_service.cmake file is specifically for "open log" functionality and only supports the ROCKSDB log state type (LOG_STATE_TYPE_RKDB). The full log state configuration with cloud variants (ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS) is handled in build_eloq_log_service.cmake.
📚 Learning: 2025-11-28T10:52:25.527Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: concourse/scripts/main.ent.bash:38-39
Timestamp: 2025-11-28T10:52:25.527Z
Learning: In the eloqdoc enterprise build (main.ent.bash), eloq_log_service is fetched separately as the resource `eloq_logservice_src` in the pipeline (main.ent.yml) and then symlinked into the data_substrate directory structure at build time using `ln -s $WORKSPACE/eloq_logservice_src eloq_log_service`. This is the intended architecture for placing eloq_log_service within data_substrate in the enterprise configuration.
Applied to files:
concourse/scripts/build_tarball.bash
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
concourse/scripts/build_tarball.bash
📚 Learning: 2025-09-25T11:58:50.446Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_log_service.cmake:116-119
Timestamp: 2025-09-25T11:58:50.446Z
Learning: The build_log_service.cmake file is specifically for "open log" functionality and only supports the ROCKSDB log state type (LOG_STATE_TYPE_RKDB). The full log state configuration with cloud variants (ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS) is handled in build_eloq_log_service.cmake.
Applied to files:
concourse/scripts/build_tarball.bash
🔇 Additional comments (4)
concourse/scripts/build_tarball.bash (4)
114-118: LGTM – Clean normalization of ELOQ_MODULE_ENABLED flag.Correct conversion of string-based environment variable to CMake boolean format.
206-231: LGTM – Correct DATA_STORE_TYPE to WITH_LOG_STATE mapping and CMake configuration.DATA_STORE_TYPE variants properly mapped to log state configuration. CMake options (EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, STATISTICS) retained as interface options per architectural requirements. OPEN_LOG_SERVICE correctly disabled.
305-313: LGTM – Comprehensive runtime linker correction strategy.Binary rpath corrections via patchelf are properly applied, with appropriate relative paths (
$ORIGIN/../libfor binaries,$ORIGINfor libraries). Conditional checks for optional binaries (host_manager, dss_server) prevent errors if not present.
1-50: Submodule and directory orchestration correctly implements intended architecture.The restructured submodule handling properly places eloq_log_service within data_substrate and conditionally includes eloqstore and raft_host_manager per PR objectives. Directory flow via pushd/popd is generally sound and recovers correctly to expected positions. Learning 1 (PR 279) regarding log_service placement is correctly implemented.
However, the critical blocker is the missing safe.directory entry for eloqstore_src—see dedicated comment above.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
concourse/scripts/build_tarball.bash (1)
79-82: Missing safe.directory entry for eloqstore_src on rhel9.Lines 79–81 configure git
safe.directoryentries for rhel9 to handle "dubious ownership" errors when submodules are checked out from external workspaces. However, the list is incomplete:eloqstore_srcis missing. Since line 41 creates a symlink toeloqstore_srcwhenDATA_STORE_TYPEisELOQDSS_ELOQSTORE, and line 44 runsgit submodule updateon it, the submodule initialization will fail on rhel9 with this data store type due to the missing safe.directory entry.Apply this diff to include the missing entry:
git config --global --add safe.directory ${WORKSPACE}/eloqdoc_src git config --global --add safe.directory ${WORKSPACE}/eloq_logservice_src git config --global --add safe.directory ${WORKSPACE}/raft_host_manager_src + git config --global --add safe.directory ${WORKSPACE}/eloqstore_src
🧹 Nitpick comments (1)
concourse/scripts/build_tarball.bash (1)
294-296: Uninitialized variable concatenation in DSS_CMAKE_ARGS.Line 295 appends to
${DSS_CMAKE_ARGS}without first initializing the variable. If the variable is unset, the expansion will result in a leading space in the string, which is harmless in this context but represents unclear intent and poor shell practice. Additionally,DSS_CMAKE_ARGSis only populated forELOQDSS_ELOQSTORE; for otherELOQDSS_*types, it remains unset and expands to empty at line 298, which is correct but inconsistent.Apply this diff to initialize the variable explicitly:
if [ -n "${DSS_TYPE}" ]; then DSS_SRC_DIR="${ELOQDOC_SRC}/src/mongo/db/modules/eloq/data_substrate/store_handler/eloq_data_store_service" cd "${DSS_SRC_DIR}" + DSS_CMAKE_ARGS="" if [ "${DATA_STORE_TYPE}" = "ELOQDSS_ELOQSTORE" ]; then - DSS_CMAKE_ARGS="${DSS_CMAKE_ARGS} -DELOQ_MODULE_ENABLED=ON" + DSS_CMAKE_ARGS="-DELOQ_MODULE_ENABLED=ON" fi mkdir -p build && cd build - cmake .. -DCMAKE_BUILD_TYPE=${BUILD_TYPE} -DWITH_DATA_STORE=${DSS_TYPE} -DUSE_ONE_ELOQDSS_PARTITION_ENABLED=OFF ${DSS_CMAKE_ARGS} + cmake .. -DCMAKE_BUILD_TYPE=${BUILD_TYPE} -DWITH_DATA_STORE=${DSS_TYPE} -DUSE_ONE_ELOQDSS_PARTITION_ENABLED=OFF ${DSS_CMAKE_ARGS:+ ${DSS_CMAKE_ARGS}}Alternatively, if you prefer to keep the append logic, initialize before the if block:
if [ -n "${DSS_TYPE}" ]; then DSS_SRC_DIR="${ELOQDOC_SRC}/src/mongo/db/modules/eloq/data_substrate/store_handler/eloq_data_store_service" cd "${DSS_SRC_DIR}" + DSS_CMAKE_ARGS="" if [ "${DATA_STORE_TYPE}" = "ELOQDSS_ELOQSTORE" ]; then DSS_CMAKE_ARGS="${DSS_CMAKE_ARGS:+${DSS_CMAKE_ARGS} }-DELOQ_MODULE_ENABLED=ON" fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
concourse/scripts/build_tarball.bash(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: concourse/scripts/main.ent.bash:38-39
Timestamp: 2025-11-28T10:52:25.527Z
Learning: In the eloqdoc enterprise build (main.ent.bash), eloq_log_service is fetched separately as the resource `eloq_logservice_src` in the pipeline (main.ent.yml) and then symlinked into the data_substrate directory structure at build time using `ln -s $WORKSPACE/eloq_logservice_src eloq_log_service`. This is the intended architecture for placing eloq_log_service within data_substrate in the enterprise configuration.
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
📚 Learning: 2025-11-28T10:52:25.527Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: concourse/scripts/main.ent.bash:38-39
Timestamp: 2025-11-28T10:52:25.527Z
Learning: In the eloqdoc enterprise build (main.ent.bash), eloq_log_service is fetched separately as the resource `eloq_logservice_src` in the pipeline (main.ent.yml) and then symlinked into the data_substrate directory structure at build time using `ln -s $WORKSPACE/eloq_logservice_src eloq_log_service`. This is the intended architecture for placing eloq_log_service within data_substrate in the enterprise configuration.
Applied to files:
concourse/scripts/build_tarball.bash
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
concourse/scripts/build_tarball.bash
📚 Learning: 2025-09-25T11:58:50.446Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_log_service.cmake:116-119
Timestamp: 2025-09-25T11:58:50.446Z
Learning: The build_log_service.cmake file is specifically for "open log" functionality and only supports the ROCKSDB log state type (LOG_STATE_TYPE_RKDB). The full log state configuration with cloud variants (ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS) is handled in build_eloq_log_service.cmake.
Applied to files:
concourse/scripts/build_tarball.bash
🔇 Additional comments (6)
concourse/scripts/build_tarball.bash (6)
25-26: Alignment with enterprise submodule sync pattern.The addition of
git submodule syncandgit submodule update --init --recursivefor the eloqdoc root module aligns with the enterprise build architecture and provides proper initialization for nested submodules. Based on learnings, this matches the intended pattern for data_substrate integration.
28-33: Data substrate and eloq_log_service setup follows established architecture.The directory traversal to
data_substrate, symlink creation foreloq_log_service, and recursive submodule initialization align with the enterprise build pattern documented in PR 279. The per-submodule sync/update flow is appropriate. Based on learnings, this matches the intended symlink architecture for placing eloq_log_service within data_substrate.
292-303: DSS build path correctly updated to data_substrate layout.The DSS source directory has been properly relocated to
data_substrate/store_handler/eloq_data_store_service, and the conditional DSS build is triggered only whenDATA_STORE_TYPEbegins withELOQDSS_. The submodule initialization for eloqstore and the conditional CMake argument passing align with the new architecture. Based on learnings from PR 279, this placement is correct.
305-313: RPATH fixes for executables are comprehensive and appropriately targeted.The patchelf invocations correctly set
$ORIGIN/../libfor the main executables (eloqdoc-cli, eloqdoc, host_manager, dss_server) and include proper existence checks before applying the RPATH. This ensures runtime library discovery is relative to the binary location, improving portability of the tarball. Combined with the library-level$ORIGINfix at line 195, the approach is consistent and sound.
322-330: Tarball naming logic correctly differentiates tagged and non-tagged builds.The tarball naming now includes OS_ID and ARCH for tagged builds (line 323), providing clear identifiers for release artifacts. The fallback to
OUT_NAMEfor non-tagged builds (line 329) preserves CI flexibility. The SQL logging at line 326 captures release metadata appropriately for tagged builds only. No issues identified.
70-70: No changes needed. Line 70 uses correct SCL syntax for RHEL 8.The
source scl_source enable gcc-toolset-11syntax on line 70 is valid and officially documented by Red Hat for persistent shell activation of gcc-toolset-11 on RHEL 8. The difference from line 63'ssource /opt/rh/devtoolset-11/enable(RHEL 7) is expected—the package naming changed from "devtoolset" to "gcc-toolset" between RHEL 7 and 8, andscl_sourceis a standard Software Collections utility function, not a custom one. Both syntaxes are correct alternatives for their respective platforms.Likely an incorrect or invalid review comment.
1514f68 to
4d0c3ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
concourse/scripts/build_tarball.bash (1)
20-22: Apply mkdir -p before chown and cd to ensure $HOME/workspace exists.If
$HOME/workspacedoes not exist, both thechownat line 20 and thecdat line 22 will fail. This issue was flagged in a prior review and remains unresolved.Apply this diff:
# Ensure workspace ownership + mkdir -p $HOME/workspace sudo chown -R $current_user $HOME/workspace 2>/dev/null || true cd $HOME/workspace
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
concourse/scripts/build_tarball.bash(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: concourse/scripts/main.ent.bash:38-39
Timestamp: 2025-11-28T10:52:25.527Z
Learning: In the eloqdoc enterprise build (main.ent.bash), eloq_log_service is fetched separately as the resource `eloq_logservice_src` in the pipeline (main.ent.yml) and then symlinked into the data_substrate directory structure at build time using `ln -s $WORKSPACE/eloq_logservice_src eloq_log_service`. This is the intended architecture for placing eloq_log_service within data_substrate in the enterprise configuration.
📚 Learning: 2025-11-28T10:52:25.527Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: concourse/scripts/main.ent.bash:38-39
Timestamp: 2025-11-28T10:52:25.527Z
Learning: In the eloqdoc enterprise build (main.ent.bash), eloq_log_service is fetched separately as the resource `eloq_logservice_src` in the pipeline (main.ent.yml) and then symlinked into the data_substrate directory structure at build time using `ln -s $WORKSPACE/eloq_logservice_src eloq_log_service`. This is the intended architecture for placing eloq_log_service within data_substrate in the enterprise configuration.
Applied to files:
concourse/scripts/build_tarball.bash
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
concourse/scripts/build_tarball.bash
📚 Learning: 2025-09-25T11:58:50.446Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_log_service.cmake:116-119
Timestamp: 2025-09-25T11:58:50.446Z
Learning: The build_log_service.cmake file is specifically for "open log" functionality and only supports the ROCKSDB log state type (LOG_STATE_TYPE_RKDB). The full log state configuration with cloud variants (ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS) is handled in build_eloq_log_service.cmake.
Applied to files:
concourse/scripts/build_tarball.bash
📚 Learning: 2025-09-25T12:24:06.434Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
Applied to files:
concourse/scripts/build_tarball.bash
🔇 Additional comments (5)
concourse/scripts/build_tarball.bash (5)
25-26: Submodule sync and data_substrate architecture align with intended design.The per-submodule sync pattern (lines 25–26, 31–32, 43–44) and symlink strategy for eloq_log_service and raft_host_manager are correctly structured and match the data_substrate layout described in prior learnings.
Also applies to: 28-37
39-46: Conditional eloqstore submodule sync and directory management are correct.The nested
cdintoeloqstoreat line 42 occurs within thepushdcontext from line 40, so thepopdat line 45 correctly unwinds the stack. Conditional guard onDATA_STORE_TYPEis appropriate.
216-216: CMake generator switch to Ninja and new build flags are appropriately configured.The switch from "Unix Makefiles" to "Ninja" improves build parallelization. The addition of
-DEXT_TX_PROC_ENABLED=ONaligns with data_substrate configuration requirements documented in prior learnings.Also applies to: 223-223
294-296: Clarify DSS_CMAKE_ARGS initialization and usage.Line 295 appends to
DSS_CMAKE_ARGSvia"${DSS_CMAKE_ARGS} -DELOQ_MODULE_ENABLED=ON", but there is no visible initialization ofDSS_CMAKE_ARGSin the script. If this is an environment variable provided by the Concourse pipeline, that should be documented. If not, consider explicitly initializing it to avoid potential hidden bugs.Verify whether
DSS_CMAKE_ARGSis:
- A pre-set environment variable from the Concourse pipeline, or
- A variable that should be initialized in this script (e.g.,
DSS_CMAKE_ARGS="").If it is a pipeline environment variable, consider adding a comment to clarify the contract.
39-46: Data_substrate layout changes are consistently integrated throughout the script.The refactoring of paths to use
data_substrate/store_handler/eloq_data_store_service, symlink linking strategy, submodule sync operations, and config file handling are all appropriately updated and consistent with the new workspace structure.Also applies to: 292-298, 316-316
5585f25 to
0274fa5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
concourse/scripts/build_tarball.bash (1)
15-24: Directory$HOME/workspaceis not created beforecd.Line 22 attempts to
cd $HOME/workspace, but the directory is never created before this operation. Line 20 only performs achownon a non-existent directory, which will silently fail due to the2>/dev/null || trueerror suppression. If$HOME/workspacedoesn't exist, thecdwill fail.A previous review flagged this and noted it was addressed, but
mkdir -p $HOME/workspaceis absent from the current code.Apply this diff to ensure the directory exists:
# Ensure workspace ownership + mkdir -p $HOME/workspace sudo chown -R $current_user $HOME/workspace 2>/dev/null || true cd $HOME/workspace
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
concourse/scripts/build_tarball.bash(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: concourse/scripts/main.ent.bash:38-39
Timestamp: 2025-11-28T10:52:25.527Z
Learning: In the eloqdoc enterprise build (main.ent.bash), eloq_log_service is fetched separately as the resource `eloq_logservice_src` in the pipeline (main.ent.yml) and then symlinked into the data_substrate directory structure at build time using `ln -s $WORKSPACE/eloq_logservice_src eloq_log_service`. This is the intended architecture for placing eloq_log_service within data_substrate in the enterprise configuration.
📚 Learning: 2025-11-28T10:52:25.527Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: concourse/scripts/main.ent.bash:38-39
Timestamp: 2025-11-28T10:52:25.527Z
Learning: In the eloqdoc enterprise build (main.ent.bash), eloq_log_service is fetched separately as the resource `eloq_logservice_src` in the pipeline (main.ent.yml) and then symlinked into the data_substrate directory structure at build time using `ln -s $WORKSPACE/eloq_logservice_src eloq_log_service`. This is the intended architecture for placing eloq_log_service within data_substrate in the enterprise configuration.
Applied to files:
concourse/scripts/build_tarball.bash
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
concourse/scripts/build_tarball.bash
📚 Learning: 2025-09-25T11:58:50.446Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_log_service.cmake:116-119
Timestamp: 2025-09-25T11:58:50.446Z
Learning: The build_log_service.cmake file is specifically for "open log" functionality and only supports the ROCKSDB log state type (LOG_STATE_TYPE_RKDB). The full log state configuration with cloud variants (ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS) is handled in build_eloq_log_service.cmake.
Applied to files:
concourse/scripts/build_tarball.bash
📚 Learning: 2025-09-25T12:24:06.434Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
Applied to files:
concourse/scripts/build_tarball.bash
🔇 Additional comments (4)
concourse/scripts/build_tarball.bash (4)
28-46: Submodule orchestration correctly structures Eloq components within data_substrate.The symlink setup for
eloq_log_service,raft_host_manager, and (conditionally)eloqstoreproperly mirrors the intended architecture. Submodule sync/update calls are correctly nested, and the conditional ELOQDSS_ELOQSTORE path adds the necessary git housekeeping for the eloqstore component. Based on learnings, this aligns with the symlink-based integration pattern used in the enterprise build.
216-231: CMake configuration correctly switches to Ninja and preserves essential flags.The change from Unix Makefiles to Ninja, along with
-DBUILD_SHARED_LIBS=ON,-DEXT_TX_PROC_ENABLED=ON,-DSTATISTICS=ON, and-DELOQ_MODULE_ENABLED(conditional), aligns with the expected build configuration. These flags are retained per the learnings from prior architecture reviews. The conditionals forELOQ_MODULE_ENABLEDand log state (WITH_LOG_STATE) are appropriately wired.
185-198: Verify the semantic change from--ignore-existingto--ignore-timesin rsync.Line 191 changes the rsync flag from
--ignore-existing(skip if destination exists) to--ignore-times(update if modification time differs). This is a semantic change that will now overwrite libraries with differing timestamps, which may be intentional for dynamic library refresh but should be explicitly verified.Confirm that forcing library updates via
--ignore-timesis the intended behavior and won't cause unintended overwrites or conflicts during parallel builds.
293-299: DSS source directory path and CMake arguments correctly updated for nested store_handler structure.
DSS_SRC_DIRnow correctly points to the nestedstore_handler/eloq_data_store_servicepath withindata_substrate. The conditional append of-DELOQ_MODULE_ENABLED=ONforELOQDSS_ELOQSTOREensures the DSS build receives the required flag when Eloqstore is the backing store. CMake invocation properly passesDSS_CMAKE_ARGSto the build.
concourse/scripts/build_tarball.bash
Outdated
| CPPDEFINES=$( [ ${ELOQ_MODULE_ENABLED} = "ON" ] && echo "ELOQ_MODULE_ENABLED" ) \ | ||
| CPPDEFINES="EXT_TX_PROC_ENABLED" \ |
There was a problem hiding this comment.
CPPDEFINES is overwritten; the ELOQ_MODULE_ENABLED flag is lost.
Line 256 conditionally sets CPPDEFINES to "ELOQ_MODULE_ENABLED" when the flag is ON, but line 257 immediately reassigns it unconditionally to "EXT_TX_PROC_ENABLED", discarding the prior value. Both flags are required when ELOQ_MODULE_ENABLED=ON.
A previous review flagged this issue and noted it was fixed in commit 5585f25, but the fix is not present in the current code. This appears to be a regression.
Apply this diff to combine both flags:
- CPPDEFINES=$( [ ${ELOQ_MODULE_ENABLED} = "ON" ] && echo "ELOQ_MODULE_ENABLED" ) \
- CPPDEFINES="EXT_TX_PROC_ENABLED" \
+ CPPDEFINES=$( [ ${ELOQ_MODULE_ENABLED} = "ON" ] && echo "ELOQ_MODULE_ENABLED EXT_TX_PROC_ENABLED" || echo "EXT_TX_PROC_ENABLED" ) \🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 256-256: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In concourse/scripts/build_tarball.bash around lines 256-257, CPPDEFINES is
being overwritten so the conditional ELOQ_MODULE_ENABLED flag is lost; change
the assignment so CPPDEFINES always contains EXT_TX_PROC_ENABLED and also
includes ELOQ_MODULE_ENABLED when ELOQ_MODULE_ENABLED=ON (for example build
CPPDEFINES by concatenating the conditional echo with EXT_TX_PROC_ENABLED,
ensuring proper quoting and spacing).
update update rebase code
012a0d9 to
fe4fadf
Compare
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.