Run CI with ELOQDSS_ELOQSTORE#424
Conversation
WalkthroughThis PR migrates the data storage backend from RocksDB Cloud S3 to EloQStore. It removes legacy RocksDB cloud configuration, adds new EloQStore-specific flags and parameters, creates a new RocksDB cloud configuration file for backward compatibility, and updates the data_substrate submodule reference. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
concourse/scripts/common.sh (1)
157-164: Minor: Inconsistent indentation.Line 157 uses tabs while other lines use spaces for indentation. Consider aligning with the file's established style.
The new EloQStore flags and the retained
txlog_rocksdb_cloud_object_store_service_urlfor the log service are properly structured.🔧 Suggested fix
nohup $PREFIX/bin/eloqdoc \ --config=./concourse/scripts/eloqdoc.yaml \ - --data_substrate_config=./concourse/scripts/data_substrate.cnf \ + --data_substrate_config=./concourse/scripts/data_substrate.cnf \ --eloq_store_cloud_endpoint=${ELOQ_AWS_S3_ENDPOINT_URL} \
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
concourse/scripts/common.shconcourse/scripts/data_substrate.cnfconcourse/scripts/data_substrate.rocksdbcloud.cnfsrc/mongo/db/modules/eloq/data_substrate
🧰 Additional context used
🧠 Learnings (7)
📓 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: 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-14T03:10:22.027Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/dbmain.cpp:0-0
Timestamp: 2025-11-14T03:10:22.027Z
Learning: In this repository, avoid defining --config via gflags in src/mongo/db/dbmain.cpp; MongoDB’s options_parser owns --config. Prefer gflags flags with a distinct prefix (e.g., --data_substrate_config) parsed before options_parser, with options_parser configured to allow unregistered options.
Applied to files:
concourse/scripts/data_substrate.rocksdbcloud.cnf
📚 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/data_substrate.cnfsrc/mongo/db/modules/eloq/data_substrateconcourse/scripts/common.sh
📚 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/data_substrate.cnfconcourse/scripts/common.sh
📚 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/common.sh
📚 Learning: 2025-09-25T12:09:01.276Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:95-115
Timestamp: 2025-09-25T12:09:01.276Z
Learning: The log states (LOG_STATE_TYPE_RKDB, LOG_STATE_TYPE_RKDB_S3, LOG_STATE_TYPE_RKDB_GCS) in the EloqDoc codebase are mutually exclusive - only one can be active at a time. The LOG_STATE_TYPE_RKDB_CLOUD macro is a helper that should only be defined for cloud variants (S3/GCS) and not when regular RKDB is active.
Applied to files:
concourse/scripts/common.sh
📚 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/common.sh
🔇 Additional comments (7)
concourse/scripts/common.sh (3)
66-76: Inconsistency:compile_and_installstill usesELOQDSS_ROCKSDB_CLOUD_S3.The non-enterprise build function still compiles with
ELOQDSS_ROCKSDB_CLOUD_S3, but thelaunch_eloqdocandlaunch_eloqdoc_fastfunctions now use--eloq_store_cloud_*flags. This mismatch could cause runtime failures when using the open-source build with the updated launch functions.Is this intentional to keep the open-source build on RocksDB Cloud while only migrating the enterprise build? If so, consider adding separate launch functions or conditional flag handling.
116-128: LGTM!The enterprise build function correctly and consistently uses
ELOQDSS_ELOQSTOREfor both the CMake configuration (line 116) and the scons environment (line 128).
181-187: LGTM!The
launch_eloqdoc_fastfunction mirrors the EloQStore flag changes made tolaunch_eloqdoc, maintaining consistency between the two launch variants.concourse/scripts/data_substrate.cnf (1)
17-19: LGTM with optional nit.The new EloQStore configuration parameters are appropriate for CI. Minor observation: setting
eloq_store_open_files_limit=1024when the comment states it's the default is redundant but provides explicit documentation of the value in use.concourse/scripts/data_substrate.rocksdbcloud.cnf (1)
1-29: This is a template file for reference; document its purpose to avoid confusion.The file documents RocksDB Cloud configuration but isn't referenced by the active
launch_eloqdocfunctions incommon.sh(which usedata_substrate.cnfinstead and pass runtime values via environment variables). While RocksDB Cloud configurations are actively used in production builds (seeconcourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/), the placeholder credentials (CHANGEME_*on lines 28-29) and hardcoded endpoint (http://127.0.0.1:9900) suggest this is a development/example template rather than a live configuration. Add a header comment clarifying its purpose so developers understand whether to use or modify it.src/mongo/db/modules/eloq/data_substrate (2)
1-1: Ensure comprehensive testing of the storage backend migration.Given the critical nature of migrating from RocksDB Cloud S3 to EloQStore, verify that:
- Integration tests cover data migration scenarios and backward compatibility
- Performance testing validates that EloQStore meets or exceeds previous performance characteristics
- Failure scenarios (connection loss, partial writes, etc.) are tested with the new storage backend
- The CI pipeline exercises both the new EloQStore path and the legacy RocksDB Cloud configuration (via
data_substrate.rocksdbcloud.cnf)
1-1: Verify submodule compatibility with EloQStore migration.This submodule update is part of a critical storage backend migration from RocksDB Cloud S3 to EloQStore. The following must be verified:
- The new submodule commit contains the necessary EloQStore implementation changes
- No breaking API changes that would affect the main repository's integration
- Compatibility with new configuration parameters (
eloq_store_cloud_endpoint,eloq_store_cloud_store_path,eloq_store_cloud_access_key,eloq_store_cloud_secret_key,eloq_store_open_files_limit,eloq_store_buffer_pool_size)- Migration paths and backward compatibility mechanisms are properly tested
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.