Converge support eloqdoc#279
Conversation
|
Warning Rate limit exceeded@githubzilla has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughConsolidates Eloq components under a new data_substrate submodule, remaps include/build paths to data_substrate, removes many Eloq CMake modules and submodule commit pointers, replaces global externs with DataSubstrate lookups, adds data_substrate runtime flags/configs, and updates CI/launch scripts and artifact configs. Changes
Sequence Diagram(s)sequenceDiagram
participant Init as Startup (dbmain / init)
participant DataSub as DataSubstrate (global)
participant Module as Eloq modules (catalog, kv engine, recovery, etc.)
participant CI as CI/Launch scripts
rect rgb(215,235,245)
Note over Init,DataSub: Initialization (new)
Init->>DataSub: DataSubstrate::InitializeGlobal(config file)
DataSub->>DataSub: create tx, store, log services
DataSub-->>Init: initialization complete
end
rect rgb(245,245,235)
Note over Module,DataSub: Runtime usage (new)
Module->>DataSub: DataSubstrate::GetGlobal()
Module->>DataSub: GetStoreHandler()/GetTxService()/GetLogServer()
DataSub-->>Module: non-owning/raw pointers/handles
end
rect rgb(235,245,235)
Note over CI,Init: CI/Launch interaction
CI->>Init: pass --data_substrate_config / env vars
CI->>DataSub: provide S3/endpoint overrides via config/template updates
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Areas needing focused review:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/mongo/db/modules/eloq/SConscript (2)
122-150: Filter out missing system includes to avoid None in CPPPATH.FindIncludePath(...) can return None; extending CPPPATH with None breaks SCons on some setups.
-env["CPPPATH"].extend( - [ - "$ELOQ_ENGINE_ROOT", - "$ELOQ_ENGINE_ROOT/src", - "$ELOQ_ENGINE_ROOT/src/base", - "$ELOQ_ENGINE_ROOT/data_substrate", - "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include", - "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include/cc", - "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include/fault", - "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include/proto", - "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include/remote", - "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include/store", - "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include/sequences", - "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/tx-log-protos", - "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/abseil-cpp", - "$ELOQ_ENGINE_ROOT/data_substrate/store_handler", - "$ELOQ_ENGINE_ROOT/data_substrate/eloq_metrics/include", - FindIncludePath("mimalloc-2.1"), - FindIncludePath("aws/core"), - FindIncludePath("aws/kinesis"), - FindIncludePath("aws/s3"), - # FindIncludePath("google/cloud/storage"), - FindIncludePath("rocksdb_cloud_header/"), - FindIncludePath("rocksdb_cloud_header/rocksdb"), - FindIncludePath("rocksdb_cloud_header/rocksdb/cloud"), - # FindIncludePath("gperftools/heap-profiler.h") - ] -) +_eloq_cpppaths = [ + "$ELOQ_ENGINE_ROOT", + "$ELOQ_ENGINE_ROOT/src", + "$ELOQ_ENGINE_ROOT/src/base", + "$ELOQ_ENGINE_ROOT/data_substrate", + "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include", + "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include/cc", + "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include/fault", + "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include/proto", + "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include/remote", + "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include/store", + "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include/sequences", + "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/tx-log-protos", + "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/abseil-cpp", + "$ELOQ_ENGINE_ROOT/data_substrate/store_handler", + "$ELOQ_ENGINE_ROOT/data_substrate/eloq_metrics/include", + FindIncludePath("mimalloc-2.1"), + FindIncludePath("aws/core"), + FindIncludePath("aws/kinesis"), + FindIncludePath("aws/s3"), + # FindIncludePath("google/cloud/storage"), + FindIncludePath("rocksdb_cloud_header/"), + FindIncludePath("rocksdb_cloud_header/rocksdb"), + FindIncludePath("rocksdb_cloud_header/rocksdb/cloud"), + # FindIncludePath("gperftools/heap-profiler.h") +] +env["CPPPATH"].extend([p for p in _eloq_cpppaths if p])Also applies to: 128-139
197-212: Don’t pull AWS libs for GCS builds (split store_deps by cloud flavor).Currently, store_deps adds AWS libs unconditionally for ROCKSDB_CLOUD; GCS builds will still try linking AWS. This will fail where AWS SDK isn’t present.
-if data_store_type == DATA_STORE_TYPE_ROCKSDB_CLOUD: - store_deps = [ - # FindLibPath("rocksdb"), - FindLibPath("aws-cpp-sdk-core"), - FindLibPath("aws-cpp-sdk-kinesis"), - FindLibPath("aws-cpp-sdk-s3"), - # FindLibPath("google_cloud_cpp_common"), - # FindLibPath("google_cloud_cpp_storage"), - FindLibPath("rocksdb-cloud-aws"), - # FindLibPath("rocksdb-cloud-gcp"), - ] +if data_store_type == DATA_STORE_TYPE_ROCKSDB_CLOUD: + if data_store_type_str == "ELOQDSS_ROCKSDB_CLOUD_S3": + store_deps = [ + FindLibPath("aws-cpp-sdk-core"), + FindLibPath("aws-cpp-sdk-kinesis"), + FindLibPath("aws-cpp-sdk-s3"), + FindLibPath("rocksdb-cloud-aws"), + ] + else: # GCS + store_deps = [ + FindLibPath("google_cloud_cpp_common"), + FindLibPath("google_cloud_cpp_storage"), + FindLibPath("rocksdb-cloud-gcp"), + ]Also applies to: 239-246
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1)
72-90: Fix broken include paths under ELOQDS conditional branches (lines 81–87).These five includes use incomplete paths and will cause compilation failures. Add the full
mongo/db/modules/eloq/data_substrate/prefix to align with lines 77–79:#if ELOQDS_RKDB_CLOUD -#include "store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.h" -#include "store_handler/eloq_data_store_service/rocksdb_config.h" +#include "mongo/db/modules/eloq/data_substrate/store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.h" +#include "mongo/db/modules/eloq/data_substrate/store_handler/eloq_data_store_service/rocksdb_config.h" #elif defined(DATA_STORE_TYPE_ELOQDSS_ROCKSDB) -#include "store_handler/eloq_data_store_service/rocksdb_config.h" -#include "store_handler/eloq_data_store_service/rocksdb_data_store_factory.h" +#include "mongo/db/modules/eloq/data_substrate/store_handler/eloq_data_store_service/rocksdb_config.h" +#include "mongo/db/modules/eloq/data_substrate/store_handler/eloq_data_store_service/rocksdb_data_store_factory.h" #elif defined(DATA_STORE_TYPE_ELOQDSS_ELOQSTORE) -#include "store_handler/eloq_data_store_service/eloq_store_data_store_factory.h" +#include "mongo/db/modules/eloq/data_substrate/store_handler/eloq_data_store_service/eloq_store_data_store_factory.h"
🧹 Nitpick comments (7)
.gitignore (1)
186-186: Consider using a more specific ignore pattern.The pattern
datais quite generic and may unintentionally ignore legitimate directories or files named "data" throughout the repository. Consider using a more specific pattern such as/data(to match only at root) or**/data/if you intend to ignore data directories at any level.src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp (1)
29-35: Path updates are fine; consider an umbrella header to reduce churn.If tx_service offers a stable aggregator (e.g., a single cc_all.h), prefer that to limit future include churn and compile time.
src/mongo/db/modules/eloq/CMakeLists.txt (1)
28-28: Scope compile definitions to targets instead of directory/global.Replace directory-wide add_definitions with target_compile_definitions on produced libs to avoid bleeding macros repo‑wide.
Apply this diff to remove the global definition:
- add_definitions(-DELOQ_MODULE_ELOQDOC)Then, after add_subdirectory(data_substrate), add target‑scoped defs (adjust if targets differ):
# Place after add_subdirectory(data_substrate) target_compile_definitions(txservice PUBLIC ELOQ_MODULE_ELOQDOC=1) target_compile_definitions(logservice PUBLIC ELOQ_MODULE_ELOQDOC=1) target_compile_definitions(data_substrate PUBLIC ELOQ_MODULE_ELOQDOC=1) target_compile_definitions(eloq-metrics PUBLIC ELOQ_MODULE_ELOQDOC=1)src/mongo/db/modules/eloq/SConscript (2)
215-221: Abseil linkage from build tree is fragile; prefer transitive linkage or bundle set consistently.Manual per-submodule LIBPATH/LIBS can cause order/duplication issues with CMake-built txservice/logservice. Consider relying on transitive links from cmake_module_libs or add the complete, consistent absl set (strings, time, synchronization, status, etc.) if you must link directly.
222-224: Add RPATH for DEST_DIR/lib and gate the debug print.Ensure runtime finds the CMake-built .so without env tweaks; also avoid noisy logs.
-print("dest_dir: ", dest_dir) +if os.environ.get("ELOQ_SCONS_DEBUG"): + print("dest_dir: ", dest_dir) dest_lib_dir = os.path.join(dest_dir, "lib") if dest_dir else None if dest_lib_dir and os.path.isdir(dest_lib_dir): env.Append(LIBPATH=[dest_lib_dir]) + # Help the loader find these at runtime. + env.Append(RPATH=[dest_lib_dir])Also applies to: 227-231
src/mongo/db/modules/eloq/src/base/eloq_table_schema.cpp (1)
248-251: Defensive check for DataSubstrate/StoreHandler before use.
Avoid UB if initialization order changes. Add invariants before deref.Example patch:
@@ - size_t offset = 0; - auto *storeHandler = DataSubstrate::GetGlobal()->GetStoreHandler(); + size_t offset = 0; + auto* ds = DataSubstrate::GetGlobal(); + invariant(ds != nullptr); + auto* storeHandler = ds->GetStoreHandler(); + invariant(storeHandler != nullptr); kv_info_ = storeHandler->DeserializeKVCatalogInfo(kv_info_str_, offset); @@ - size_t offset = 0; - auto *storeHandler = DataSubstrate::GetGlobal()->GetStoreHandler(); + size_t offset = 0; + auto* ds = DataSubstrate::GetGlobal(); + invariant(ds != nullptr); + auto* storeHandler = ds->GetStoreHandler(); + invariant(storeHandler != nullptr); kv_info_ = storeHandler->DeserializeKVCatalogInfo(kv_info, offset);If this code can run pre-engine init (e.g., tooling/tests), prefer uassert with an InternalError instead of hard invariant.
Also applies to: 304-308
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1)
585-591: Shutdown path: handle uninitialized DataSubstrate.
GetGlobal() may be null in error paths; guard before Shutdown().- DataSubstrate::GetGlobal()->Shutdown(); + if (auto* ds = DataSubstrate::GetGlobal()) { + ds->Shutdown(); + } _txService = nullptr; _logServer = nullptr;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
.gitignore(1 hunks).gitmodules(1 hunks)src/mongo/SConscript(1 hunks)src/mongo/base/object_pool.h(1 hunks)src/mongo/db/dbmain.cpp(2 hunks)src/mongo/db/modules/eloq/CMakeLists.txt(2 hunks)src/mongo/db/modules/eloq/SConscript(6 hunks)src/mongo/db/modules/eloq/data_substrate(1 hunks)src/mongo/db/modules/eloq/eloq_metrics(0 hunks)src/mongo/db/modules/eloq/log_service(0 hunks)src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_key.h(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_table_schema.cpp(3 hunks)src/mongo/db/modules/eloq/src/base/eloq_table_schema.h(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_util.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_cursor.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_cursor.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_global_options.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_global_options.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_index.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_index.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(7 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.h(2 hunks)src/mongo/db/modules/eloq/src/eloq_record_store.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_record_store.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp(3 hunks)src/mongo/db/modules/eloq/src/eloq_recovery_unit.h(1 hunks)src/mongo/db/modules/eloq/store_handler(0 hunks)src/mongo/db/modules/eloq/tx_service(0 hunks)src/mongo/transport/service_executor_coroutine.h(1 hunks)
💤 Files with no reviewable changes (4)
- src/mongo/db/modules/eloq/eloq_metrics
- src/mongo/db/modules/eloq/log_service
- src/mongo/db/modules/eloq/tx_service
- src/mongo/db/modules/eloq/store_handler
🧰 Additional context used
🧬 Code graph analysis (2)
src/mongo/db/dbmain.cpp (1)
src/mongo/db/commands/mr.cpp (1)
config(1414-1414)
src/mongo/db/modules/eloq/src/eloq_kv_engine.h (1)
src/mongo/db/modules/eloq/src/base/eloq_util.h (1)
txservice(43-43)
🔇 Additional comments (28)
src/mongo/db/modules/eloq/src/eloq_global_options.h (1)
29-29: LGTM! Header path update aligns with data_substrate refactoring.The include path has been correctly updated to reference the new data_substrate location.
.gitmodules (1)
1-3: Verify the submodule configuration is correct.The submodule path has been updated to
data_substratewhile continuing to use thetx_service.gitrepository URL. Please confirm this is the intended configuration and that the data_substrate submodule content is compatible with the tx_service repository.src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
35-37: LGTM! Include paths correctly updated.All three include directives have been properly updated to reference the new data_substrate location.
src/mongo/db/modules/eloq/src/eloq_recovery_unit.h (1)
39-43: LGTM! Include paths correctly updated.All five include directives have been consistently updated to reference the new data_substrate location.
src/mongo/db/modules/eloq/src/base/eloq_key.h (1)
33-33: LGTM! Header path update is correct.The include path has been properly updated to reference the new data_substrate location.
src/mongo/base/object_pool.h (1)
7-7: LGTM! Include path correctly updated.The include directive has been properly updated to reference the new data_substrate location.
src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h (1)
26-26: LGTM! Header path update is correct.The include path has been properly updated to reference the new data_substrate location.
src/mongo/db/modules/eloq/src/eloq_index.h (1)
26-26: Include path migration looks correct.Consistent with data_substrate refactor; no interface/ABI changes here.
src/mongo/transport/service_executor_coroutine.h (1)
10-10: Cannot fully verify header presence in sandbox environment.Type signature usage confirmed intact—
txservice::ConcurrentQueueWSize<Task>is used consistently for_taskQueueand_resumeQueue. However, the file search produced no results, making it unclear whether the header exists, is generated, or located in a submodule. Manual verification needed to confirm the new include path resolves correctly and that no stale copies exist elsewhere in the codebase.src/mongo/db/modules/eloq/src/eloq_kv_engine.h (3)
192-193: Add ownership documentation to raw pointers or initialize explicitly in header.The pointers are obtained from
DataSubstrate::GetGlobal()(lines 183–184) and are non-owning (owned by the global singleton). However, the header lacks initialization and ownership documentation. Add a clarifying comment consistent with the pattern ineloq_recovery_unit.h:private: - txservice::TxService *_txService; - txlog::LogServer *_logServer; + txservice::TxService* _txService; // not owned (from DataSubstrate) + txlog::LogServer* _logServer; // not owned (from DataSubstrate)No changes to
shutdownTxService()or destructor are needed—they correctly callShutdown()on the global without deleting, andcleanShutdown()sets them tonullptrafter global shutdown.
31-36: Build flag consistency confirmed; includes are live and necessary.The conditional includes properly declare the
txlog::LogServertype on line 193. Build flag consistency is maintained across both CMake (default ON) and SConscript (default "1"), with both supporting toggling and tested via separate CI scripts (build_tarball_open.bash and build_tarball.bash). The include paths are not dead code—both branches are required for successful compilation. No changes needed.
25-27: Remove unused includes from header; types are not class members.The includes for
eloq_catalog_factory.handeloq_log_agent.hare not used ineloq_kv_engine.h. NeitherMongoCatalogFactorynorMongoLogAgentappear in any member declarations, method signatures, or type references in the header. These types are used only in the.cppimplementation (e.g., instantiation at line 126 ineloq_kv_engine.cpp), so the includes belong there, not in the header. Removing them reduces unnecessary coupling.src/mongo/db/modules/eloq/SConscript (3)
101-104: Log service include path toggle looks fine; ensure libs match the header path.If OPEN_LOG_SERVICE=0, you include eloq_log_service headers but still link "logservice" later. Verify the pairings are consistent.
178-180: ELOQSTORE include path addition looks OK.
233-235: CMake targets are properly configured and discoverable.The codebase evidence confirms the review concern is already addressed:
- CMakeLists.txt defines all four libraries as CMake install targets with proper installation rules to the lib/ directory
- The SConscript already includes defensive code that conditionally adds
DEST_DIR/libto LIBPATH before appending the library names- Library names in the SConscript match exactly with the CMakeLists.txt install targets
The code is correct and requires no changes.
src/mongo/db/modules/eloq/src/eloq_record_store.h (1)
25-25: LGTM! Include path updated to data_substrate structure.The include path correctly reflects the new data_substrate module organization.
src/mongo/db/modules/eloq/src/eloq_cursor.h (1)
24-25: LGTM! Include paths migrated to data_substrate.Both tx_service includes correctly updated to the new data_substrate path structure.
src/mongo/db/modules/eloq/src/eloq_cursor.cpp (1)
25-26: LGTM! Include paths updated consistently.Implementation file includes properly migrated to match the data_substrate structure.
src/mongo/db/modules/eloq/src/base/eloq_table_schema.h (1)
40-44: LGTM! Comprehensive include path migration.All tx_service headers correctly updated to the data_substrate module structure.
src/mongo/db/dbmain.cpp (1)
53-53: Consider error handling for flag parsing.The gflags parsing call has no error handling, and the third parameter
falsemeans flags remain in argv for subsequent processing. Verify:
- Whether any validation or error handling is needed if flag parsing fails
- If leaving flags in argv (false parameter) could cause issues with MongoDB's existing argument processing in mongoDbMain
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (1)
45-50: LGTM! Include paths consistently migrated.All tx_service and store_handler includes properly updated to the data_substrate structure. The removal of the global storeHandler extern declaration (mentioned in the AI summary) aligns with the shift to using DataSubstrate singleton pattern.
src/mongo/db/modules/eloq/src/base/eloq_util.h (3)
36-41: LGTM! Includes updated to data_substrate structure.Include paths properly migrated and data_substrate.h added to support the singleton pattern.
55-57: New filtering logic for sequence tables.The code now filters out
sequence_table_name_svfrom the discovered tables. Verify:
- Is this filtering intentional and documented? (behavior change)
- Should this filtering happen before or after checking success?
- Are there other special table names that should be filtered?
49-50: Verify DataSubstrate singleton initialization and null safety.Based on code analysis, DataSubstrate::InitializeGlobal() is called in eloq_kv_engine.cpp:181, and GetStoreHandler() is invoked at multiple sites (eloq_util.h:49, eloq_recovery_unit.cpp:517/648, eloq_table_schema.cpp:249/306) without explicit null checks. Verify that:
- InitializeGlobal() is guaranteed to execute before any code path reaching the GetStoreHandler() call sites
- GetStoreHandler() has contractual guarantees it cannot return nullptr, or is protected by implicit assertions
- Thread safety and timing are correct for all usage patterns
src/mongo/db/modules/eloq/src/base/eloq_table_schema.cpp (1)
43-48: Includes updated to DataSubstrate layout look good.
Paths align with usage below. No issues spotted.src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (1)
41-48: Header migration to data_substrate is consistent.
No concerns with the new include set.src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (2)
134-136: Review comment is based on a non-manifesting concern and should be disregarded.The review warns about complications from DECLARE_string usage elsewhere, but a comprehensive search of the codebase finds no DECLARE_string(data_substrate_config) anywhere. Additionally, the flag (FLAGS_data_substrate_config) is referenced only at line 181 of the same file, within the same namespace mongo. The suggested refactoring addresses a theoretical ODR/DECLARE risk that does not actually exist in the codebase.
Likely an incorrect or invalid review comment.
125-131: No multiple definitions detected — review concern unfounded.Verification confirms these symbols are defined only once in
eloq_kv_engine.cppwith no duplicates across other translation units, no extern declarations in headers, and no ODR/link violations. The code is correct as written.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (2)
517-518: Guard DataSubstrate access against null pointers (duplicate concern).This unguarded access to
DataSubstrate::GetGlobal()->GetStoreHandler()risks null pointer dereference during early initialization or shutdown. The past review comment correctly identified this pattern and provided guards.
648-650: Guard DataSubstrate access against null pointers (duplicate concern).This unguarded access to
DataSubstrate::GetGlobal()->GetStoreHandler()risks null pointer dereference. The past review comment correctly identified this pattern and should be applied here as well as increateTable.src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (2)
180-184: Guard DataSubstrate initialization and validate service pointers (duplicate concern).The initialization sequence lacks validation of
InitializeGlobalsuccess and null checks on retrieved service pointers. The past review comment correctly identified this pattern and provided appropriate guards.
209-209: Guard against null _txService before constructing RecoveryUnit (duplicate concern).Constructing
EloqRecoveryUnitwith a null_txServicewill cause failures. The past review comment correctly identified this issue and suggested adding a null check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/mongo/db/modules/eloq/src/base/eloq_table_schema.cpp(3 hunks)src/mongo/db/modules/eloq/src/base/eloq_util.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_global_options.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(7 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.h(2 hunks)src/mongo/db/modules/eloq/src/eloq_record_store.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/mongo/db/modules/eloq/src/eloq_global_options.cpp
- src/mongo/db/modules/eloq/src/base/eloq_table_schema.cpp
- src/mongo/db/modules/eloq/src/eloq_kv_engine.h
🔇 Additional comments (6)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (1)
40-40: LGTM! Include path migration aligns with data_substrate reorganization.The updated include paths correctly reflect the new data_substrate module structure.
Also applies to: 47-50
src/mongo/db/modules/eloq/src/base/eloq_util.h (1)
36-41: LGTM! Include path updates support data_substrate migration.The updated includes correctly reflect the new module structure and provide necessary dependencies.
src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (1)
35-35: LGTM! Include path updates align with data_substrate migration.The updated includes correctly reflect the new module structure.
Also applies to: 43-47
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (3)
36-59: LGTM! Comprehensive include path migration to data_substrate.The updated includes correctly reflect the new module structure across multiple conditional compilation paths.
Also applies to: 77-79, 122-122
126-130: LGTM! Factory registration supports data_substrate integration.The global factory pointers enable data_substrate to access MongoDB-specific catalog and system handlers.
135-135: Verify DataSubstrate::InitializeGlobal("") safely handles empty configuration string.The
data_substrate_configflag defaults to an empty string and is passed unconditionally toDataSubstrate::InitializeGlobal()at line 181 during engine initialization. Since the DataSubstrate implementation appears to be external to this repository, manual verification is required to confirm:
- Whether empty string configuration is valid/supported
- If not valid, whether the method fails explicitly with a clear error message or silently uses unsafe defaults
- Whether sensible fallback behavior exists for empty configuration
githubzilla
force-pushed
the
converge_support_eloqdoc
branch
from
9ba8cdf to
296d898
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mongo/db/modules/eloq/src/eloq_kv_engine.h (1)
20-22: Add missing standard headers used in this header.Types used below require these includes; without them, builds may fail unpredictably depending on transitive includes.
#include <memory> #include <vector> +#include <set> +#include <thread> +#include <deque> +#include <future> +#include <condition_variable> +#include <mutex> +#include <atomic> +#include <functional>
♻️ Duplicate comments (7)
src/mongo/db/modules/eloq/CMakeLists.txt (1)
59-74: Verify install() commands reference correct target names.This concern was flagged in a prior review: the
install(TARGETS ...)commands must reference the actual CMake target names created by the added subdirectory (line 55). The current code referencestxservice,logservice,data_substrate, andeloq-metrics. Per the AI summary, these targets were renamed from their_sharedversions in this PR.However, confirm that
data_substrate/CMakeLists.txtactually creates targets with these exact names—not just OUTPUT_NAME aliases, which wouldn't work withinstall(TARGETS ...).Run the following script to verify the target definitions:
#!/bin/bash # Search for add_library calls in data_substrate subdirectory find . -path '*/data_substrate/CMakeLists.txt' -o -path '*/data_substrate/**/CMakeLists.txt' | while read file; do if [ -f "$file" ]; then echo "=== Found: $file ===" grep -nP 'add_library\s*\(\s*(txservice|logservice|data_substrate|eloq-metrics|eloq_metrics|datastore|eloq-metrics|txservice_shared|logservice_shared|datastore_shared|eloq_metrics_shared)' "$file" | head -20 fi done # Also check for OUTPUT_NAME properties that might alias these targets echo "" echo "=== Checking for OUTPUT_NAME properties ===" find . -path '*/data_substrate/**' -name 'CMakeLists.txt' | xargs grep -n 'OUTPUT_NAME' 2>/dev/null | head -20src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (1)
517-518: Add defensive null pointer guards to DataSubstrate access in both createTable and updateTable.Both locations call
DataSubstrate::GetGlobal()->GetStoreHandler()without null checks, creating potential for null dereference under race conditions or incomplete initialization. This was flagged in previous reviews and remains unaddressed.Apply guards consistently in both functions:
In createTable (line 517):
- auto* storeHandler = DataSubstrate::GetGlobal()->GetStoreHandler(); - std::string kvInfo = storeHandler->CreateKVCatalogInfo(&tempSchema); + auto* ds = DataSubstrate::GetGlobal(); + invariant(ds != nullptr); + auto* storeHandler = ds->GetStoreHandler(); + invariant(storeHandler != nullptr); + std::string kvInfo = storeHandler->CreateKVCatalogInfo(&tempSchema); + invariant(!kvInfo.empty());In updateTable (lines 648-650):
- auto* storeHandler = DataSubstrate::GetGlobal()->GetStoreHandler(); - std::string new_kv_info = - storeHandler->CreateNewKVCatalogInfo(tableName, currentTableSchema, alterTableInfo); + auto* ds = DataSubstrate::GetGlobal(); + invariant(ds != nullptr); + auto* storeHandler = ds->GetStoreHandler(); + invariant(storeHandler != nullptr); + std::string new_kv_info = + storeHandler->CreateNewKVCatalogInfo(tableName, currentTableSchema, alterTableInfo); + invariant(!new_kv_info.empty());Based on learnings (past review comments).
Also applies to: 648-650
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (3)
181-185: Validate DataSubstrate initialization and service pointers.The code assumes
DataSubstrate::InitializeGlobal()succeeds and thatGetGlobal()returns valid service pointers, with no validation. If initialization fails or returns nullptrs, subsequent operations will crash. This was flagged in previous reviews.Add defensive validation:
LOG(INFO) << "Standalone mode: Initializing data substrate..."; DataSubstrate::InitializeGlobal(FLAGS_data_substrate_config); - - _logServer = DataSubstrate::GetGlobal()->GetLogServer(); - _txService = DataSubstrate::GetGlobal()->GetTxService(); + + auto* ds = DataSubstrate::GetGlobal(); + uassert(ErrorCodes::InternalError, "DataSubstrate initialization failed", ds != nullptr); + + _logServer = ds->GetLogServer(); + _txService = ds->GetTxService(); + uassert(ErrorCodes::InternalError, "TxService initialization failed", _txService != nullptr); + uassert(ErrorCodes::InternalError, "LogServer initialization failed", _logServer != nullptr);Based on learnings (past review comments).
210-210: Guard against null _txService in newRecoveryUnit.Constructing
EloqRecoveryUnitwith a null_txServicepointer will lead to crashes when the recovery unit attempts to use it. This was flagged in previous reviews.Add a guard before construction:
RecoveryUnit* EloqKVEngine::newRecoveryUnit() { MONGO_LOG(1) << "EloqKVEngine::newRecoveryUnit"; + invariant(_txService != nullptr, "Cannot create RecoveryUnit: TxService is not initialized"); return new EloqRecoveryUnit(_txService); }Based on learnings (past review comments).
589-591: Guard DataSubstrate access during shutdown.Calling
DataSubstrate::GetGlobal()->Shutdown()without null checks risks crashes if DataSubstrate was never initialized or already torn down. This was flagged in previous reviews.Add defensive guard:
void EloqKVEngine::cleanShutdown() { MONGO_LOG(0) << "EloqKVEngine::cleanShutdown"; - DataSubstrate::GetGlobal()->Shutdown(); + auto* ds = DataSubstrate::GetGlobal(); + if (ds != nullptr) { + ds->Shutdown(); + } _txService = nullptr; _logServer = nullptr; }Based on learnings (past review comments).
src/mongo/db/modules/eloq/SConscript (2)
247-254: Guard verbose prints and avoid KeyError on env lookup.Same concern as earlier review; wrap under a flag and use env.get.
-print("CPPDEFINES: ", env["CPPDEFINES"]) -print("CPPPATH: ", env["CPPPATH"]) -print("LIBPATH: ", env["LIBPATH"]) -print("LIBS: ", env["LIBS"]) -print("LINKFLAGS: ", env["LINKFLAGS"]) -print("eloq_dependencies: ", eloq_dependencies) -print("MALLOC:", env['MONGO_ALLOCATOR']) +if os.environ.get("ELOQ_SCONS_DEBUG"): + print("CPPDEFINES: ", env.get("CPPDEFINES")) + print("CPPPATH: ", env.get("CPPPATH")) + print("LIBPATH: ", env.get("LIBPATH")) + print("LIBS: ", env.get("LIBS")) + print("LINKFLAGS: ", env.get("LINKFLAGS")) + print("eloq_dependencies: ", eloq_dependencies) + print("MALLOC:", env.get('MONGO_ALLOCATOR', '<unset>'))
58-59: Fix leading space in CPPDEFINES macro.Leading space breaks preprocessor checks.
-env.Append(CPPDEFINES=[" ELOQ_MODULE_ELOQDOC"]) +env.Append(CPPDEFINES=["ELOQ_MODULE_ELOQDOC"])
🧹 Nitpick comments (6)
src/mongo/db/modules/eloq/src/eloq_kv_engine.h (1)
25-27: Consider trimming unused includes to reduce rebuilds.If eloq_catalog_factory.h and eloq_log_agent.h are no longer referenced in this header, move them to the .cpp or remove. Keeps public header light.
src/mongo/db/modules/eloq/SConscript (5)
128-140: Use AppendUnique for CPPPATH to avoid dupes and order churn.Direct list.extend can create duplicates; SCons provides AppendUnique.
- [ + CPPPATH=[ "$ELOQ_ENGINE_ROOT", "$ELOQ_ENGINE_ROOT/src", "$ELOQ_ENGINE_ROOT/src/base", "$ELOQ_ENGINE_ROOT/data_substrate", "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include", "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include/cc", "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include/fault", "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include/proto", "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include/remote", "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include/store", "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/include/sequences", "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/tx-log-protos", "$ELOQ_ENGINE_ROOT/data_substrate/tx_service/abseil-cpp", "$ELOQ_ENGINE_ROOT/data_substrate/store_handler", "$ELOQ_ENGINE_ROOT/data_substrate/eloq_metrics/include", FindIncludePath("mimalloc-2.1"), @@ - ] -) + ])And change the call to:
-env["CPPPATH"].extend( - CPPPATH=[ ... ] -) +env.AppendUnique(**CPPPATH)
215-223: Brittle Abseil linkage; prefer consuming via CMake targets or verify existence.Hardcoding absl subdirs makes builds fragile across platforms/configs. Prefer linking transitively through data_substrate/txservice targets, or at least guard with existence checks.
Minimal hardening:
-env.Append(LIBPATH=["$ELOQ_ENGINE_ROOT/build/data_substrate/tx_service/abseil-cpp/absl/base"]) -env.Append(LIBS=["absl_throw_delegate", "absl_log_severity", "absl_raw_logging_internal"]) -env.Append(LIBPATH=["$ELOQ_ENGINE_ROOT/build/data_substrate/tx_service/abseil-cpp/absl/hash"]) -env.Append(LIBS=["absl_hash"]) -env.Append(LIBPATH=["$ELOQ_ENGINE_ROOT/build/data_substrate/tx_service/abseil-cpp/absl/container"]) -env.Append(LIBS=["absl_raw_hash_set"]) +# Prefer libs exposed by cmake_module_libs; if direct linking is required, verify the libs exist: +absl_candidates = [ + ("$ELOQ_ENGINE_ROOT/build/data_substrate/tx_service/abseil-cpp/absl/base", ["absl_throw_delegate","absl_log_severity","absl_raw_logging_internal"]), + ("$ELOQ_ENGINE_ROOT/build/data_substrate/tx_service/abseil-cpp/absl/hash", ["absl_hash"]), + ("$ELOQ_ENGINE_ROOT/build/data_substrate/tx_service/abseil-cpp/absl/container", ["absl_raw_hash_set"]), +] +for libpath, libs in absl_candidates: + if os.path.isdir(env.Subst(libpath)): + env.AppendUnique(LIBPATH=[libpath]) + env.AppendUnique(LIBS=libs)
227-227: Guard diagnostic print with a debug flag.This spams build logs.
-print("dest_dir: ", dest_dir) +if os.environ.get("ELOQ_SCONS_DEBUG"): + print("dest_dir: ", dest_dir)
37-40: Don’t print every library candidate path during discovery.This is very noisy; gate behind the same debug flag.
- print("candidate:",candidate) + if os.environ.get("ELOQ_SCONS_DEBUG"): + print("candidate:", candidate)
6-6: Remove unused variable or use it.
link_model = get_option('link-model')is currently unused. Drop it or apply where needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
.gitignore(1 hunks).gitmodules(1 hunks)src/mongo/SConscript(1 hunks)src/mongo/base/object_pool.h(1 hunks)src/mongo/db/dbmain.cpp(2 hunks)src/mongo/db/modules/eloq/CMakeLists.txt(2 hunks)src/mongo/db/modules/eloq/SConscript(6 hunks)src/mongo/db/modules/eloq/data_substrate(1 hunks)src/mongo/db/modules/eloq/eloq_metrics(0 hunks)src/mongo/db/modules/eloq/log_service(0 hunks)src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_key.h(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_table_schema.cpp(3 hunks)src/mongo/db/modules/eloq/src/base/eloq_table_schema.h(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_util.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_cursor.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_cursor.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_global_options.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_global_options.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_index.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_index.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(7 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.h(2 hunks)src/mongo/db/modules/eloq/src/eloq_record_store.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_record_store.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp(3 hunks)src/mongo/db/modules/eloq/src/eloq_recovery_unit.h(1 hunks)src/mongo/db/modules/eloq/store_handler(0 hunks)src/mongo/db/modules/eloq/tx_service(0 hunks)src/mongo/transport/service_executor_coroutine.h(1 hunks)
💤 Files with no reviewable changes (4)
- src/mongo/db/modules/eloq/log_service
- src/mongo/db/modules/eloq/store_handler
- src/mongo/db/modules/eloq/tx_service
- src/mongo/db/modules/eloq/eloq_metrics
✅ Files skipped from review due to trivial changes (1)
- src/mongo/db/modules/eloq/data_substrate
🚧 Files skipped from review as they are similar to previous changes (14)
- .gitignore
- src/mongo/db/modules/eloq/src/eloq_global_options.h
- src/mongo/db/modules/eloq/src/eloq_record_store.h
- src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp
- src/mongo/transport/service_executor_coroutine.h
- src/mongo/db/modules/eloq/src/eloq_cursor.h
- src/mongo/db/modules/eloq/src/eloq_cursor.cpp
- src/mongo/db/modules/eloq/src/base/eloq_util.h
- src/mongo/base/object_pool.h
- src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h
- src/mongo/db/modules/eloq/src/base/eloq_table_schema.h
- src/mongo/db/modules/eloq/src/eloq_global_options.cpp
- src/mongo/SConscript
- src/mongo/db/dbmain.cpp
🔇 Additional comments (7)
src/mongo/db/modules/eloq/CMakeLists.txt (2)
28-28: Add ELOQ_MODULE_ELOQDOC preprocessor definition.Simple compile-time flag addition for module identification.
55-55: Consolidate build wiring into data_substrate subdirectory.Replacing extensive modular configuration with a single
add_subdirectory(data_substrate)simplifies the build system and aligns with the PR's data_substrate restructuring.src/mongo/db/modules/eloq/src/eloq_index.h (1)
26-26: Include path is correct but requires git submodule initialization.The include path update at line 26 is part of the ongoing data_substrate migration and is correctly formatted. However, the
data_substratedirectory is a git submodule that needs to be initialized.Ensure developers understand they must run:
git submodule update --init src/mongo/db/modules/eloq/data_substrateThe change itself is valid—other files in the codebase already use the same new include paths. The submodule is properly configured in
.gitmodules.src/mongo/db/modules/eloq/src/eloq_kv_engine.h (1)
31-36: Include path rewires to data_substrate look good.Paths align with OPEN_LOG_SERVICE split and tx_service migration. No issues from this change.
src/mongo/db/modules/eloq/SConscript (3)
101-103: Include path switch to data_substrate is correct.OPEN_LOG_SERVICE gating and CPPPATH additions look consistent.
178-179: Path update for ELOQSTORE looks right.Matches the new data_substrate layout.
233-234: Library list change OK; verify link order if symbols resolve late.If txservice/logservice depend on data_substrate, keeping data_substrate earlier is fine; otherwise, you may need to reorder.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/mongo/db/dbmain.cpp (1)
36-39: Do not claim --config via gflags; conflicts with MongoDB options_parser. Rename or move.Defining gflags::FLAGS_config will collide with MongoDB’s existing “config” handling and may pre‑consume/ambiguate the flag before options_parser runs. Prefer either:
- Move registration to MongoDB’s options_parser, or
- Rename to an Eloq‑scoped flag (e.g., --eloq-config) to avoid conflict.
Option A (rename only, minimal change):
-#include <gflags/gflags.h> -DEFINE_string(config, "", "eloqdoc configuration file"); +#include <gflags/gflags.h> +DEFINE_string(eloq_config, "", "eloqdoc configuration file");If you choose Option B (use options_parser), also remove the gflags parse in main (see below).
Run to confirm existing --config is owned by MongoDB:
#!/bin/bash rg -n 'config' src/mongo/db/mongod_options.cpp src/mongo/util/options_parser/ -C3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/mongo/db/dbmain.cpp(2 hunks)src/mongo/db/modules/eloq/data_substrate(1 hunks)src/mongo/db/modules/eloq/src/eloq_options_init.cpp(1 hunks)src/mongo/util/options_parser/options_parser.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mongo/db/modules/eloq/data_substrate
🔇 Additional comments (3)
src/mongo/util/options_parser/options_parser.cpp (2)
511-515: LGTM: Comment reformattingThe comment reformatting for
addCompositionsandaddConstraintsfunctions is purely cosmetic with no functional impact.Also applies to: 599-602
717-718: Address silent option failures and clarify data_substrate option handling.The addition of
.allow_unregistered()silently discards unrecognized options without any forwarding, logging, or validation. Option typos (e.g.,--storageEngin,--aut) now fail silently instead of raising errors, and the comment suggests data-substrate options are handled—but they are not. Unregistered options are neither stored in the variables_map nor added to the environment; they are discarded after parsing completes.Issues:
- Silent failures: Typos in mongo option names pass parsing without effect
- Security risk: Mistyped security options create false security
- Misleading comment: "Allow unregistered options from data substrate" implies they're forwarded, but no such forwarding exists
- Missing validation: No mechanism catches or reports unregistered options
Required actions:
- Clarify whether data-substrate options should be handled by a separate parser and implement actual forwarding if needed
- If unregistered options must be allowed, add logging/warnings to surface them for debugging
- Update or remove the misleading comment
- Verify registered mongo options are validated to prevent common typos
src/mongo/db/dbmain.cpp (1)
53-58: Avoid pre‑parsing argv with gflags before mongoDbMain; causes ordering issues and platform drift.Pre‑parsing here can interfere with MongoDB’s options parsing and differs from the Windows path (which doesn’t parse gflags at all). Either remove this or guard it tightly.
Option B (if moving to options_parser): remove gflags pre‑parse
- // When compiled as standalone mode, allow unrecognized flags - // to be passed through to mongod - gflags::AllowCommandLineReparsing(); - // Allow all mongod-specific flags to pass through without error - gflags::SetCommandLineOption("undefok", "*"); - GFLAGS_NAMESPACE::ParseCommandLineNonHelpFlags(&argc, &argv, false);If keeping gflags for an Eloq‑scoped flag, at minimum align platforms and restrict scope, e.g. gate with a build flag and ensure Windows path mirrors behavior.
Please confirm the expected behavior for:
- Linux: flags parsed by both gflags and options_parser
- Windows: flags parsed only by options_parser
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mongo/db/server_options.h(1 hunks)src/mongo/db/server_options_server_helpers.cpp(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/mongo/db/server_options.h
🔇 Additional comments (2)
src/mongo/db/server_options_server_helpers.cpp (2)
573-578: LGTM!The validation logic for
adaptiveThreadNumis correct and the error message clearly describes the requirement.
132-146: Both concerns are valid and warrant attention.The review comment is substantively correct:
Grammatical error confirmed: The code snippet shows "must equals to", which is incorrect. The proper form is "must equal" (without "to"). This error appears on lines 143-144 as cited.
Default value concern is reasonable: The description states the parameter "must equal core_number of data substrate", yet the default is 0. While 0 could intentionally represent "not configured", this semantic tension should be clarified. Either the description should explicitly state that 0 means "use system default", or the default should be reconsidered.
The suggested grammatical fix is correct and should be applied. The developer should clarify the intended semantics of the default value through either updated documentation or code comments.
githubzilla
force-pushed
the
converge_support_eloqdoc
branch
from
46a53f8 to
e25b5e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (15)
src/mongo/db/dbmain.cpp (1)
36-59: Still breaking Mongo's--configoption
DefiningDEFINE_string(config, …)and callingParseCommandLineNonHelpFlags()pre-consumes--config, so by the timemongoDbMain()invokes the MongoDB options_parser the flag is gone. That regresses the built-in config file handling (and behaves differently on Windows, where we don’t run gflags at all). Please drop the gflags registration and integrate through Mongo’s existing options_parser (or rename the flag so it doesn’t collide), otherwise Linux builds can’t read their config files.src/mongo/db/server_options_server_helpers.cpp (1)
140-145: Error text still contradicts actual validation
We only checkreservedThreadNum >= 1, yet the message still says “must equals to core_number of data substrate.” That’s both ungrammatical and misleading. Either enforce the equality against the substrate core count or update the wording to match reality (e.g., “must be at least 1”).Also applies to: 582-585
src/mongo/db/modules/eloq/CMakeLists.txt (1)
59-74: Verify install target names match actual CMake targets.The past review flagged that these install() commands may reference OUTPUT_NAME values rather than actual CMake target names. If the targets are defined as
txservice_shared,logservice_shared,datastore_shared, andeloq_metrics_sharedin data_substrate/CMakeLists.txt, these install commands will fail during configuration.Run this script to verify the actual target names:
#!/bin/bash # Search for add_library definitions of these targets in data_substrate rg -nP 'add_library\s*\(\s*(txservice|logservice|data_substrate|datastore|eloq-metrics|eloq_metrics)' src/mongo/db/modules/eloq/data_substrate/ -A2 -B1 # Also check for set_target_properties OUTPUT_NAME rg -nP 'set_target_properties.*OUTPUT_NAME' src/mongo/db/modules/eloq/data_substrate/ -A1 -B1If the actual target names are
*_shared, update lines 59-74:
txservice→txservice_sharedlogservice→logservice_shareddata_substrate→datastore_sharedeloq-metrics→eloq_metrics_sharedsrc/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (2)
519-520: Add null pointer guards to createTable's DataSubstrate access.The chained call
DataSubstrate::GetGlobal()->GetStoreHandler()lacks defensive checks. If either returnsnullptr, this will crash with a null pointer dereference.Apply defensive guards:
- auto* storeHandler = DataSubstrate::GetGlobal()->GetStoreHandler(); - std::string kvInfo = storeHandler->CreateKVCatalogInfo(&tempSchema); + auto* ds = DataSubstrate::GetGlobal(); + invariant(ds != nullptr); + auto* storeHandler = ds->GetStoreHandler(); + invariant(storeHandler != nullptr); + std::string kvInfo = storeHandler->CreateKVCatalogInfo(&tempSchema);
650-652: Apply the same null pointer guards to updateTable's DataSubstrate access.Consistency: this location requires the same defensive checks as createTable above.
Apply defensive guards:
- auto* storeHandler = DataSubstrate::GetGlobal()->GetStoreHandler(); - std::string new_kv_info = - storeHandler->CreateNewKVCatalogInfo(tableName, currentTableSchema, alterTableInfo); + auto* ds = DataSubstrate::GetGlobal(); + invariant(ds != nullptr); + auto* storeHandler = ds->GetStoreHandler(); + invariant(storeHandler != nullptr); + std::string new_kv_info = + storeHandler->CreateNewKVCatalogInfo(tableName, currentTableSchema, alterTableInfo);src/mongo/db/modules/eloq/src/base/eloq_table_schema.cpp (2)
249-250: Add null pointer guards to MongoTableSchema constructor's DataSubstrate access.The chained call
DataSubstrate::GetGlobal()->GetStoreHandler()lacks defensive checks and can crash if either returnsnullptr.Apply defensive guards:
- auto* storeHandler = DataSubstrate::GetGlobal()->GetStoreHandler(); - kv_info_ = storeHandler->DeserializeKVCatalogInfo(kv_info_str_, offset); + auto* ds = DataSubstrate::GetGlobal(); + invariant(ds != nullptr); + auto* storeHandler = ds->GetStoreHandler(); + invariant(storeHandler != nullptr); + kv_info_ = storeHandler->DeserializeKVCatalogInfo(kv_info_str_, offset);
306-307: Apply the same null pointer guards to SetKVCatalogInfo.Consistency: this location requires the same defensive checks as the constructor above.
Apply defensive guards:
- auto* storeHandler = DataSubstrate::GetGlobal()->GetStoreHandler(); - kv_info_ = storeHandler->DeserializeKVCatalogInfo(kv_info, offset); + auto* ds = DataSubstrate::GetGlobal(); + invariant(ds != nullptr); + auto* storeHandler = ds->GetStoreHandler(); + invariant(storeHandler != nullptr); + kv_info_ = storeHandler->DeserializeKVCatalogInfo(kv_info, offset);src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (3)
181-185: Validate DataSubstrate initialization; fail fast on errors.The initialization and subsequent
GetGlobal()calls assume success without validation. IfInitializeGlobalfails orGetGlobal()returnsnullptr, the engine will crash when accessing_txServiceor_logServer.Add validation:
LOG(INFO) << "Standalone mode: Initializing data substrate..."; DataSubstrate::InitializeGlobal(FLAGS_data_substrate_config); - -_logServer = DataSubstrate::GetGlobal()->GetLogServer(); -_txService = DataSubstrate::GetGlobal()->GetTxService(); + +auto* ds = DataSubstrate::GetGlobal(); +invariant(ds != nullptr); +_logServer = ds->GetLogServer(); +_txService = ds->GetTxService(); +invariant(_txService != nullptr); +invariant(_logServer != nullptr);
208-210: Guard newRecoveryUnit against null _txService.Constructing an
EloqRecoveryUnitwith a null_txServicepointer will cause crashes during recovery unit operations.Add defensive check:
RecoveryUnit* EloqKVEngine::newRecoveryUnit() { MONGO_LOG(1) << "EloqKVEngine::newRecoveryUnit"; + invariant(_txService != nullptr); return new EloqRecoveryUnit(_txService); }
589-591: Guard DataSubstrate access during shutdown.Calling
DataSubstrate::GetGlobal()->Shutdown()without null checks risks crashes if DataSubstrate was never initialized or already shut down.Add defensive guard:
void EloqKVEngine::cleanShutdown() { MONGO_LOG(0) << "EloqKVEngine::cleanShutdown"; - DataSubstrate::GetGlobal()->Shutdown(); + auto* ds = DataSubstrate::GetGlobal(); + if (ds != nullptr) { + ds->Shutdown(); + } _txService = nullptr; _logServer = nullptr; }src/mongo/db/modules/eloq/src/eloq_kv_engine.h (1)
195-196: Initialize non-owning raw pointers to nullptr.Switching from
unique_ptrto raw pointers without initialization leaves them with indeterminate values, risking crashes if accessed before DataSubstrate initialization completes.Initialize to nullptr and consider deleting copy/move operations to prevent lifetime bugs:
+public: + EloqKVEngine(const EloqKVEngine&) = delete; + EloqKVEngine& operator=(const EloqKVEngine&) = delete; + EloqKVEngine(EloqKVEngine&&) = delete; + EloqKVEngine& operator=(EloqKVEngine&&) = delete; + private: - txservice::TxService* _txService; - txlog::LogServer* _logServer; + // Non-owning, provided by DataSubstrate; not deleted here. + txservice::TxService* _txService{nullptr}; + txlog::LogServer* _logServer{nullptr}; std::string _dbPath;src/mongo/db/modules/eloq/src/eloq_options_init.cpp (1)
30-47: Critical issue remains unresolved: Disabled startup hooks break eloq options initialization.This code change was previously flagged as a critical issue. Commenting out all three startup option hooks completely disables the eloqOptions initialization system, which breaks code that depends on
eloqGlobalOptionsfields (e.g.,ccProtocol,enableIOuring,raftlogAsyncFsyncreferenced in eloq_recovery_unit.cpp and eloq_kv_engine.cpp).The core problems remain:
- No option registration: Users cannot configure eloq options via command-line or config files
- Dead code: The commented hooks and related option processing code serve no purpose
- Silent failures: Code will use default values instead of user-specified configuration
As recommended in the previous review, either:
- Complete the migration by ensuring
FLAGS_data_substrate_configfully replaces all individual eloqGlobalOptions fields, then remove this dead code- Restore and update these hooks to properly initialize eloqGlobalOptions
Additionally, consider adding a TODO/FIXME comment explaining why this code is commented out if this is intentional work-in-progress.
src/mongo/db/modules/eloq/SConscript (3)
57-58: Trim the CPPDEFINES entry.The new define still starts with a leading space, so the compiler sees
-D ELOQ_MODULE_ELOQDOCinstead of the intended macro, breaking feature guards. Please drop the leading whitespace.-env.Append(CPPDEFINES=[" ELOQ_MODULE_ELOQDOC"]) +env.Append(CPPDEFINES=["ELOQ_MODULE_ELOQDOC"])
212-213: Filter out missing dependency paths.
FindLibPathcan returnNone; passing those straight through keeps poisoningSYSLIBDEPSand SCons will choke when it tries to treatNoneas a filesystem node. Filter the combined list before assigning it.-eloq_dependencies = base_deps + store_deps +eloq_dependencies = [lib for lib in (base_deps + store_deps) if lib]
246-253: Gate the debug prints and avoid KeyError.These prints still spam every build and
env['MONGO_ALLOCATOR']will crash when unset. Please guard them behind a debug flag and rely onenv.get(...)with a default.-print("CPPDEFINES: ", env["CPPDEFINES"]) -print("CPPPATH: ", env["CPPPATH"]) -print("LIBPATH: ", env["LIBPATH"]) -print("LIBS: ", env["LIBS"]) -print("LINKFLAGS: ", env["LINKFLAGS"]) -print("eloq_dependencies: ", eloq_dependencies) -print("MALLOC:", env['MONGO_ALLOCATOR']) +if os.environ.get("ELOQ_SCONS_DEBUG"): + print("CPPDEFINES: ", env.get("CPPDEFINES")) + print("CPPPATH: ", env.get("CPPPATH")) + print("LIBPATH: ", env.get("LIBPATH")) + print("LIBS: ", env.get("LIBS")) + print("LINKFLAGS: ", env.get("LINKFLAGS")) + print("eloq_dependencies: ", eloq_dependencies) + print("MALLOC:", env.get("MONGO_ALLOCATOR", "<unset>"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
.gitignore(1 hunks).gitmodules(1 hunks)src/mongo/SConscript(1 hunks)src/mongo/base/object_pool.h(1 hunks)src/mongo/db/dbmain.cpp(2 hunks)src/mongo/db/modules/eloq/CMakeLists.txt(2 hunks)src/mongo/db/modules/eloq/SConscript(6 hunks)src/mongo/db/modules/eloq/data_substrate(1 hunks)src/mongo/db/modules/eloq/eloq_metrics(0 hunks)src/mongo/db/modules/eloq/log_service(0 hunks)src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_key.h(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_table_schema.cpp(3 hunks)src/mongo/db/modules/eloq/src/base/eloq_table_schema.h(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_util.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_cursor.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_cursor.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_global_options.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_global_options.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_index.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_index.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(7 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.h(2 hunks)src/mongo/db/modules/eloq/src/eloq_options_init.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_record_store.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_record_store.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp(3 hunks)src/mongo/db/modules/eloq/src/eloq_recovery_unit.h(1 hunks)src/mongo/db/modules/eloq/store_handler(0 hunks)src/mongo/db/modules/eloq/tx_service(0 hunks)src/mongo/db/server_options.h(1 hunks)src/mongo/db/server_options_server_helpers.cpp(2 hunks)src/mongo/transport/service_executor_coroutine.h(1 hunks)src/mongo/util/options_parser/options_parser.cpp(3 hunks)
💤 Files with no reviewable changes (4)
- src/mongo/db/modules/eloq/tx_service
- src/mongo/db/modules/eloq/log_service
- src/mongo/db/modules/eloq/store_handler
- src/mongo/db/modules/eloq/eloq_metrics
✅ Files skipped from review due to trivial changes (3)
- src/mongo/db/modules/eloq/data_substrate
- src/mongo/db/modules/eloq/src/eloq_record_store.h
- src/mongo/base/object_pool.h
🚧 Files skipped from review as they are similar to previous changes (13)
- .gitignore
- src/mongo/db/modules/eloq/src/eloq_index.h
- src/mongo/db/modules/eloq/src/base/eloq_util.h
- src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp
- src/mongo/SConscript
- .gitmodules
- src/mongo/db/modules/eloq/src/eloq_global_options.h
- src/mongo/db/modules/eloq/src/eloq_recovery_unit.h
- src/mongo/db/modules/eloq/src/eloq_cursor.cpp
- src/mongo/db/server_options.h
- src/mongo/transport/service_executor_coroutine.h
- src/mongo/db/modules/eloq/src/eloq_global_options.cpp
- src/mongo/util/options_parser/options_parser.cpp
🧰 Additional context used
🧠 Learnings (3)
📚 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:
src/mongo/db/modules/eloq/src/eloq_kv_engine.cppsrc/mongo/db/modules/eloq/SConscriptsrc/mongo/db/modules/eloq/CMakeLists.txt
📚 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:
src/mongo/db/modules/eloq/src/eloq_kv_engine.cppsrc/mongo/db/modules/eloq/SConscript
📚 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:
src/mongo/db/modules/eloq/SConscriptsrc/mongo/db/modules/eloq/CMakeLists.txt
🧬 Code graph analysis (2)
src/mongo/db/dbmain.cpp (1)
src/mongo/db/commands/mr.cpp (1)
config(1414-1414)
src/mongo/db/modules/eloq/src/eloq_kv_engine.h (1)
src/mongo/db/modules/eloq/src/base/eloq_util.h (1)
txservice(43-43)
🔇 Additional comments (6)
src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h (1)
26-26: Include path migration is consistent across the codebase; verify build succeeds with submodule populated.The include path change from
tx_service/include/todata_substrate/tx_service/include/is complete and consistent—no old references remain, and botheloq_catalog_factory.handeloq_table_schema.huse the new path. Thedata_substratesubmodule is properly configured in.gitmodules. However, since the submodule is not initialized in this environment, verify that the build completes successfully when the submodule is fully populated.src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
35-37: The include path refactoring is correct and complete.The three include path updates in lines 35-37 are verified as correct. All references to the old
mongo/db/modules/eloq/tx_service/include/paths have been successfully replaced with the newmongo/db/modules/eloq/data_substrate/tx_service/include/paths across the codebase. The data_substrate submodule is properly configured in.gitmodulesto pull from the externaltx_servicerepository, which will provide these headers at build time.src/mongo/db/modules/eloq/src/eloq_cursor.h (1)
24-25: Include path migration looks good.The updated paths correctly point to the data_substrate layout for tx_execution.h and tx_request.h.
src/mongo/db/modules/eloq/src/base/eloq_table_schema.h (1)
40-44: Include path updates are correct.All five tx_service headers now correctly reference the data_substrate path layout.
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (2)
40-40: Store handler include path updated correctly.The kv_store.h path now points to data_substrate/store_handler as expected.
47-50: Transaction service include paths updated correctly.All four tx_service headers (catalog_key_record.h, tx_key.h, tx_request.h, type.h) now correctly reference the data_substrate layout.
githubzilla
force-pushed
the
converge_support_eloqdoc
branch
from
00e0b3b to
cd88230
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (17)
src/mongo/db/modules/eloq/SConscript (3)
57-57: Trim the macro name.The leading space in
" ELOQ_MODULE_ELOQDOC"means the compiler sees a macro literally named with a space prefix, so#ifdef ELOQ_MODULE_ELOQDOCwill never fire. Please drop the space.
214-220: AvoidNoneentries in SYSLIBDEPS.
FindLibPath(...)can returnNone; appending that straight intoeloq_dependenciessendsNoneto SCons and breaks dependency scanning. Filter falsy values before assigning.
246-253: Gate the debug prints and use safe lookups.These unconditional prints spam every build, and
env['MONGO_ALLOCATOR']can still throw when unset. Wrap them behind a debug flag/env var and switch toenv.get(...)with defaults.src/mongo/db/modules/eloq/CMakeLists.txt (1)
59-74: Install target names remain incorrect (duplicate of past review).The install() commands still reference non-existent target names. As previously flagged, OUTPUT_NAME changes the output filename but not the CMake target name itself, so these install commands will fail during configuration.
src/mongo/db/dbmain.cpp (1)
51-56: gflags integration conflicts with MongoDB's options_parser (duplicate of past review).As previously flagged, gflags
ParseCommandLineNonHelpFlags()consumes command-line arguments beforemongoDbMainand MongoDB'srunGlobalInitializers()execute. Even withundefok="*"andAllowCommandLineReparsing(), this design introduces ordering dependencies and potential conflicts with MongoDB's existing option handling.src/mongo/db/modules/eloq/src/base/eloq_util.h (1)
49-50: Null pointer guards still missing on DataSubstrate access (duplicate of past review).The chained call
DataSubstrate::GetGlobal()->GetStoreHandler()lacks defensive checks. If eitherGetGlobal()orGetStoreHandler()returns nullptr (due to initialization races or shutdown), this will crash with a null pointer dereference.Apply defensive guards:
inline bool GetAllTables(std::vector<std::string>& tables, const std::function<void()>* yieldFuncPtr, const std::function<void()>* resumeFuncPtr) { - auto* storeHandler = DataSubstrate::GetGlobal()->GetStoreHandler(); + auto* ds = DataSubstrate::GetGlobal(); + if (ds == nullptr) { + return false; + } + auto* storeHandler = ds->GetStoreHandler(); + if (storeHandler == nullptr) { + return false; + } bool success = storeHandler->DiscoverAllTableNames(tables, yieldFuncPtr, resumeFuncPtr);src/mongo/db/modules/eloq/src/base/eloq_table_schema.cpp (2)
249-250: Add null pointer guards to DataSubstrate access (duplicate of past review).The chained call
DataSubstrate::GetGlobal()->GetStoreHandler()lacks null checks. Crashes will occur if either returns nullptr during initialization or shutdown races.Apply this defensive pattern:
+ auto* ds = DataSubstrate::GetGlobal(); + if (ds == nullptr) { + // Handle error: throw or return with clear diagnostic + uasserted(ErrorCodes::InternalError, "DataSubstrate not initialized"); + } - auto* storeHandler = DataSubstrate::GetGlobal()->GetStoreHandler(); + auto* storeHandler = ds->GetStoreHandler(); + if (storeHandler == nullptr) { + uasserted(ErrorCodes::InternalError, "StoreHandler not available"); + } kv_info_ = storeHandler->DeserializeKVCatalogInfo(kv_info_str_, offset);
306-307: Apply the same null pointer guards here (duplicate of past review).Consistency: this location also requires defensive checks identical to the constructor.
Apply this diff:
+ auto* ds = DataSubstrate::GetGlobal(); + if (ds == nullptr) { + uasserted(ErrorCodes::InternalError, "DataSubstrate not initialized"); + } - auto* storeHandler = DataSubstrate::GetGlobal()->GetStoreHandler(); + auto* storeHandler = ds->GetStoreHandler(); + if (storeHandler == nullptr) { + uasserted(ErrorCodes::InternalError, "StoreHandler not available"); + } kv_info_ = storeHandler->DeserializeKVCatalogInfo(kv_info, offset);src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (2)
519-520: Null pointer guards missing in createTable (duplicate of past review).Despite past review comments marked as "addressed," the current code at lines 519-520 still lacks defensive null checks for
DataSubstrate::GetGlobal()->GetStoreHandler().Apply defensive guards:
+ auto* ds = DataSubstrate::GetGlobal(); + if (ds == nullptr) { + return {ErrorCodes::InternalError, "DataSubstrate not initialized"}; + } - auto* storeHandler = DataSubstrate::GetGlobal()->GetStoreHandler(); + auto* storeHandler = ds->GetStoreHandler(); + if (storeHandler == nullptr) { + return {ErrorCodes::InternalError, "StoreHandler not available"}; + } std::string kvInfo = storeHandler->CreateKVCatalogInfo(&tempSchema);
650-652: Null pointer guards missing in updateTable (duplicate of past review).Apply the same defensive pattern here for consistency with createTable.
Apply this diff:
+ auto* ds = DataSubstrate::GetGlobal(); + if (ds == nullptr) { + return {ErrorCodes::InternalError, "DataSubstrate not initialized"}; + } - auto* storeHandler = DataSubstrate::GetGlobal()->GetStoreHandler(); + auto* storeHandler = ds->GetStoreHandler(); + if (storeHandler == nullptr) { + return {ErrorCodes::InternalError, "StoreHandler not available"}; + } std::string new_kv_info = storeHandler->CreateNewKVCatalogInfo(tableName, currentTableSchema, alterTableInfo);src/mongo/db/modules/eloq/src/eloq_options_init.cpp (1)
30-47: Commented hooks may break eloqGlobalOptions access.The previous review identified that disabling these startup hooks leaves
eloqGlobalOptionsfields unpopulated, while other code (eloq_kv_engine.cpp lines 168, 175, 177) still accesses them. The review was marked as addressed, but the hooks remain commented in this diff.Either:
- The migration to
data_substrate_configis complete and this commented code should be removed entirely, or- These hooks need to be restored/updated to populate the fields still being accessed
Run this script to verify whether eloqGlobalOptions fields are still accessed:
#!/bin/bash # Verify eloqGlobalOptions field usage across the codebase rg -nP '\beloqGlobalOptions\.(enableIOuring|raftlogAsyncFsync|ccProtocol)\b' --type=cpp -C2src/mongo/db/modules/eloq/src/eloq_kv_engine.h (1)
195-196: Past review concern about raw pointers appears unaddressed.The previous review flagged that these raw pointers lack default initialization and should have copy/move operations deleted to prevent lifetime bugs. However, the code still shows:
- No default initialization (should be
= nullptr)- No deleted copy/move constructors/operators
If commits 31a3dd2 to e25b5e3 addressed this elsewhere, those changes aren't visible in the current diff.
Apply this diff if the issue wasn't actually resolved:
class EloqKVEngine final : public KVEngine { +public: + EloqKVEngine(const EloqKVEngine&) = delete; + EloqKVEngine& operator=(const EloqKVEngine&) = delete; + EloqKVEngine(EloqKVEngine&&) = delete; + EloqKVEngine& operator=(EloqKVEngine&&) = delete; + // friend class EloqRecordStore; public: @@ private: - txservice::TxService* _txService; - txlog::LogServer* _logServer; + // Non-owning, provided by DataSubstrate; not deleted here. + txservice::TxService* _txService{nullptr}; + txlog::LogServer* _logServer{nullptr};src/mongo/db/server_options_server_helpers.cpp (2)
139-145: Fix grammatical error and resolve default value inconsistency.Two issues with this option declaration:
- Grammatical error: "must equals to" should be "must equal"
- Default value of 0 conflicts with validation at line 582 requiring the value be at least 1
Apply this diff to fix both issues:
options ->addOptionChaining("storage.eloq.reservedThreadNum", "eloqReservedThreadNum", moe::Int, - "set the thread num for coroutine service executor mode must equals to " + "set the thread num for coroutine service executor mode, must equal " "core_number of data substrate") - .setDefault(moe::Value(0)); + .setDefault(moe::Value(1));
580-586: Grammatical error and misleading validation message.The error message contains the grammatical error "must equal to" (should be "must equal") and claims a constraint that isn't enforced. The code validates
reservedThreadNum >= 1, but the message states it "must equal core_number of data substrate."Apply this diff:
if (serverGlobalParams.reservedThreadNum < 1) { return Status(ErrorCodes::BadValue, - "eloqReservedThreadNum has to be at least 1, must equal to core_number " + "eloqReservedThreadNum has to be at least 1, must equal core_number " "of data substrate"); }Note: If equality with data substrate core_number is truly required, that validation must occur during data substrate initialization, not here.
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (3)
181-191: Missing null checks and error handling after DataSubstrate initialization.The previous review identified that this initialization assumes success without validation. The code should:
- Check return value/status of
InitializeGlobal(or catch exceptions)- Verify
GetGlobal()returns non-null- Verify
GetLogServer()andGetTxService()return non-nullThe past review was marked as addressed in commits 0ab680c to 296d898, but no defensive checks are visible in the current code.
Apply this diff if the issue wasn't actually resolved:
log() << "Standalone mode: Initializing data substrate..."; DataSubstrate::InitializeGlobal(FLAGS_data_substrate_config); + + auto* ds = DataSubstrate::GetGlobal(); + uassert(ErrorCodes::InternalError, "DataSubstrate failed to initialize", ds != nullptr); + + _logServer = ds->GetLogServer(); + _txService = ds->GetTxService(); + + uassert(ErrorCodes::InternalError, "Failed to get LogServer from DataSubstrate", _logServer != nullptr); + uassert(ErrorCodes::InternalError, "Failed to get TxService from DataSubstrate", _txService != nullptr); - _logServer = DataSubstrate::GetGlobal()->GetLogServer(); - _txService = DataSubstrate::GetGlobal()->GetTxService();
210-213: Missing null check before constructing EloqRecoveryUnit.The previous review identified that
newRecoveryUnitshould verify_txServiceis non-null before constructing the recovery unit. If initialization failed, this would construct an EloqRecoveryUnit with a null service pointer, leading to crashes.Apply this diff:
RecoveryUnit* EloqKVEngine::newRecoveryUnit() { MONGO_LOG(1) << "EloqKVEngine::newRecoveryUnit"; + invariant(_txService != nullptr, "TxService not initialized"); return new EloqRecoveryUnit(_txService); }
595-601: Missing null check before calling DataSubstrate::Shutdown().The previous review identified that calling
DataSubstrate::GetGlobal()->Shutdown()without null checks risks crashes if DataSubstrate was never initialized or already shut down.Apply this diff:
void EloqKVEngine::cleanShutdown() { MONGO_LOG(0) << "EloqKVEngine::cleanShutdown"; - DataSubstrate::GetGlobal()->Shutdown(); + auto* ds = DataSubstrate::GetGlobal(); + if (ds != nullptr) { + ds->Shutdown(); + } _txService = nullptr; _logServer = nullptr; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.gitignore(1 hunks).gitmodules(1 hunks)src/mongo/SConscript(1 hunks)src/mongo/base/object_pool.h(1 hunks)src/mongo/db/dbmain.cpp(2 hunks)src/mongo/db/modules/eloq/CMakeLists.txt(2 hunks)src/mongo/db/modules/eloq/SConscript(6 hunks)src/mongo/db/modules/eloq/cmake/build_data_store.cmake(0 hunks)src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake(0 hunks)src/mongo/db/modules/eloq/cmake/build_eloq_metrics.cmake(0 hunks)src/mongo/db/modules/eloq/cmake/build_log_service.cmake(0 hunks)src/mongo/db/modules/eloq/cmake/build_tx_service.cmake(0 hunks)src/mongo/db/modules/eloq/cmake/compile_protos.cmake(0 hunks)src/mongo/db/modules/eloq/cmake/find_dependencies.cmake(0 hunks)src/mongo/db/modules/eloq/data_substrate(1 hunks)src/mongo/db/modules/eloq/eloq_metrics(0 hunks)src/mongo/db/modules/eloq/log_service(0 hunks)src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_key.h(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_table_schema.cpp(3 hunks)src/mongo/db/modules/eloq/src/base/eloq_table_schema.h(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_util.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_cursor.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_cursor.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_global_options.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_global_options.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_index.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_index.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(7 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.h(2 hunks)src/mongo/db/modules/eloq/src/eloq_options_init.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_record_store.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_record_store.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp(3 hunks)src/mongo/db/modules/eloq/src/eloq_recovery_unit.h(1 hunks)src/mongo/db/modules/eloq/store_handler(0 hunks)src/mongo/db/modules/eloq/tx_service(0 hunks)src/mongo/db/server_options.h(1 hunks)src/mongo/db/server_options_server_helpers.cpp(2 hunks)src/mongo/transport/service_executor_coroutine.h(1 hunks)src/mongo/util/options_parser/options_parser.cpp(3 hunks)
💤 Files with no reviewable changes (11)
- src/mongo/db/modules/eloq/eloq_metrics
- src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake
- src/mongo/db/modules/eloq/cmake/build_log_service.cmake
- src/mongo/db/modules/eloq/tx_service
- src/mongo/db/modules/eloq/log_service
- src/mongo/db/modules/eloq/store_handler
- src/mongo/db/modules/eloq/cmake/build_tx_service.cmake
- src/mongo/db/modules/eloq/cmake/build_data_store.cmake
- src/mongo/db/modules/eloq/cmake/build_eloq_metrics.cmake
- src/mongo/db/modules/eloq/cmake/find_dependencies.cmake
- src/mongo/db/modules/eloq/cmake/compile_protos.cmake
🚧 Files skipped from review as they are similar to previous changes (12)
- src/mongo/db/modules/eloq/src/eloq_recovery_unit.h
- src/mongo/db/modules/eloq/src/base/eloq_key.h
- src/mongo/transport/service_executor_coroutine.h
- src/mongo/db/modules/eloq/src/eloq_cursor.h
- src/mongo/base/object_pool.h
- src/mongo/db/modules/eloq/data_substrate
- src/mongo/db/modules/eloq/src/eloq_cursor.cpp
- .gitmodules
- src/mongo/SConscript
- src/mongo/db/modules/eloq/src/base/eloq_table_schema.h
- src/mongo/db/modules/eloq/src/eloq_index.h
- src/mongo/db/modules/eloq/src/eloq_global_options.cpp
🧰 Additional context used
🧠 Learnings (3)
📚 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:
src/mongo/db/modules/eloq/src/eloq_kv_engine.cppsrc/mongo/db/modules/eloq/SConscriptsrc/mongo/db/modules/eloq/CMakeLists.txt
📚 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:
src/mongo/db/modules/eloq/src/eloq_kv_engine.cppsrc/mongo/db/modules/eloq/SConscript
📚 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:
src/mongo/db/modules/eloq/SConscriptsrc/mongo/db/modules/eloq/CMakeLists.txt
🔇 Additional comments (10)
src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp (1)
29-35: Include path refactoring is complete and verified as correct.All seven include paths in
src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp(lines 29-35) have been properly updated to use the newdata_substrate/tx_service/include/format. The verification confirms:
- The new directory structure exists at
src/mongo/db/modules/eloq/data_substrate/- No old-style
tx_service/include/paths remain in the codebase—all references now includedata_substrate- The refactoring is consistently applied across all 19 files that reference these headers
src/mongo/db/modules/eloq/CMakeLists.txt (2)
28-28: LGTM: Compile-time feature flag added.The
ELOQ_MODULE_ELOQDOCdefinition appropriately identifies this build variant.
55-55: LGTM: data_substrate subdirectory integration.Adding the
data_substratesubdirectory consolidates the new module structure.src/mongo/db/server_options.h (1)
82-83: LGTM: Inline documentation clarifies enableCoroutine behavior.The added comments helpfully document that this option is always true and that EloqDoc depends on coroutine support.
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (1)
40-50: LGTM: Include path migration to data_substrate.The updated include paths align with the broader repository reorganization moving Eloq components under the data_substrate structure.
src/mongo/util/options_parser/options_parser.cpp (1)
717-718: Unregistered options are silently discarded with no downstream validation or logging.The verification reveals the review concerns are valid. While ~100 eloq storage options are registered in eloq_global_options.cpp, the
addBoostVariablesToEnvironment()function only processes registered options. Any unregistered options passed through.allow_unregistered()are silently dropped from the variables_map without validation, logging, or reporting. This creates a validation gap where typos or unintended flags pass parse time with no error.The code does not meet the requested verification points:
- Data substrate options ARE registered, but unregistered options bypassing registration are never consumed/validated downstream in parseCommandLine
- No logging or reporting of remaining unregistered options occurs
- No evidence that security implications have been assessed in relation to this relaxed parsing
src/mongo/db/modules/eloq/src/eloq_kv_engine.h (1)
32-39: Include path migration looks correct.The updated paths consistently reflect the data_substrate reorganization while preserving the conditional compilation logic for OPEN_LOG_SERVICE.
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (3)
37-60: Include path migration is consistent.The updated include paths correctly reflect the data_substrate reorganization across metrics, store_handler, and tx_service components.
126-131: Global linkage for data substrate integration.This pattern registers MongoDB-specific implementations (catalog factory and system handler) with data_substrate via global pointers. While globals can have initialization order issues, this appears to be an intentional integration strategy for the data substrate to access MongoDB functionality.
135-136: Data substrate configuration flag defined correctly.The gflags-based configuration provides a centralized way to configure data substrate, replacing individual Eloq startup options. The empty default suggests configuration is required.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/mongo/db/modules/eloq/src/eloq_global_options.cpp(2 hunks)src/mongo/db/modules/eloq/src/eloq_global_options.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(7 hunks)src/mongo/db/modules/eloq/src/eloq_options_init.cpp(1 hunks)src/mongo/db/server_options_server_helpers.cpp(2 hunks)src/mongo/transport/service_entry_point_impl.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/mongo/db/modules/eloq/src/eloq_options_init.cpp
🧰 Additional context used
🧠 Learnings (2)
📚 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:
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
📚 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:
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/eloq_global_options.cpp (1)
src/mongo/db/server_options.h (1)
bootstrap(87-270)
🔇 Additional comments (2)
src/mongo/transport/service_entry_point_impl.cpp (1)
75-76: LGTM! Helpful debug log for coroutine initialization.The debug log appropriately outputs the configuration parameters that determine whether the coroutine executor is created. The log level (1) ensures it only appears when debug logging is enabled, and the placement right before the conditional check (line 77) makes it easy to correlate the logged values with the executor creation decision.
src/mongo/db/server_options_server_helpers.cpp (1)
561-566: LGTM! Validation logic is correct and clear.The validation properly enforces the minimum constraint, and the error message is grammatically correct and user-friendly. The code correctly reads the option value and stores it in the global params.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
concourse/scripts/common.sh (2)
171-191: Consider refactoring or removinglaunch_eloqdoc_fastto align with the new CLI-driven approach.The
launch_eloqdoc_fastfunction still uses the legacy pattern of sed-based YAML patching (lines 181-182) and older flag names (--eloqRocksdbCloudBucketName, etc.), whilelaunch_eloqdochas been refactored to use the modern CLI flag approach. This architectural inconsistency creates maintenance overhead and makes the codebase harder to understand.Consider either:
- Refactoring
launch_eloqdoc_fastto use the same CLI flag pattern aslaunch_eloqdoc, or- Removing it if it's no longer needed
157-169: Consider validating config file existence before launch.The function references two configuration files (
store_rocksdb_cloud.yamlanddata_substrate.cnf) but doesn't verify they exist before launching eloqdoc. Adding existence checks would provide earlier, clearer feedback if files are missing.Example validation to add after line 156:
if [ ! -f "./concourse/scripts/store_rocksdb_cloud.yaml" ]; then echo "Error: store_rocksdb_cloud.yaml not found" exit 1 fi if [ ! -f "./concourse/scripts/data_substrate.cnf" ]; then echo "Error: data_substrate.cnf not found" exit 1 fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
concourse/scripts/common.sh(1 hunks)concourse/scripts/data_substrate.cnf(1 hunks)concourse/scripts/store_rocksdb_cloud.yaml(1 hunks)
🔇 Additional comments (2)
concourse/scripts/store_rocksdb_cloud.yaml (1)
12-12: LGTM!The addition of
adaptiveThreadNum: 1to the network configuration is syntactically correct and aligns with the broader data_substrate migration described in the PR objectives.concourse/scripts/data_substrate.cnf (1)
1-23: LGTM! Test/development configuration is appropriate.This new data_substrate configuration file is well-structured with clear sections for local resources, cluster settings, and cloud storage. The hardcoded MinIO credentials (
minioadmin/minioadmin) and localhost endpoint (http://127.0.0.1:9900) are appropriate for the test/CI environment context, as confirmed by the MinIO setup incommon.sh(lines 8-14).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
concourse/scripts/common.sh (1)
155-166: Fix inconsistent indentation (duplicate of prior review).The command flags continue to use mixed tabs and spaces, creating visual inconsistency. Lines 157, 160, 161, 164, 165 should use spaces to align with line 156 and other flag lines.
Apply this diff to standardize indentation:
nohup LD_LIBRARY_PATH=$PREFIX/lib/:${LD_LIBRARY_PATH} $PREFIX/bin/eloqdoc \ --config=./concourse/scripts/store_rocksdb_cloud.yaml \ - --data_substrate_config=./concourse/scripts/data_substrate.cnf \ + --data_substrate_config=./concourse/scripts/data_substrate.cnf \ --rocksdb_cloud_bucket_name="$bucket_name" \ --rocksdb_cloud_bucket_prefix="$bucket_prefix" \ - --rocksdb_cloud_object_path="dss" \ - --rocksdb_cloud_s3_endpoint_url=${MINIO_ENDPOINT} \ + --rocksdb_cloud_object_path="dss" \ + --rocksdb_cloud_s3_endpoint_url=${MINIO_ENDPOINT} \ --txlog_rocksdb_cloud_bucket_name="$bucket_name" \ --txlog_rocksdb_cloud_bucket_prefix="$bucket_prefix" \ - --txlog_rocksdb_cloud_object_path="txlog" \ - --txlog_rocksdb_cloud_endpoint_url=${MINIO_ENDPOINT} \ + --txlog_rocksdb_cloud_object_path="txlog" \ + --txlog_rocksdb_cloud_endpoint_url=${MINIO_ENDPOINT} \ &>$PREFIX/log/eloqdoc.out &
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
concourse/scripts/common.sh(5 hunks)concourse/scripts/data_substrate.cnf(1 hunks)concourse/scripts/main.bash(1 hunks)concourse/scripts/main.ent.bash(1 hunks)concourse/scripts/pr.ent.bash(1 hunks)concourse/scripts/store_rocksdb_cloud.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- concourse/scripts/data_substrate.cnf
🧰 Additional context used
🪛 Gitleaks (8.29.0)
concourse/scripts/pr.ent.bash
[high] 6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 7-7: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (2)
concourse/scripts/common.sh (1)
179-190: Apply same indentation fix tolaunch_eloqdoc_fast().The same tab/space inconsistency appears in the
launch_eloqdoc_fast()function. Apply the same standardization to lines 181, 184, 185, 188, 189:nohup LD_LIBRARY_PATH=$PREFIX/lib/:${LD_LIBRARY_PATH} $PREFIX/bin/eloqdoc \ --config=./concourse/scripts/store_rocksdb_cloud.yaml \ - --data_substrate_config=./concourse/scripts/data_substrate.cnf \ + --data_substrate_config=./concourse/scripts/data_substrate.cnf \ --rocksdb_cloud_bucket_name="$bucket_name" \ --rocksdb_cloud_bucket_prefix="$bucket_prefix" \ - --rocksdb_cloud_object_path="dss" \ - --rocksdb_cloud_s3_endpoint_url=${MINIO_ENDPOINT} \ + --rocksdb_cloud_object_path="dss" \ + --rocksdb_cloud_s3_endpoint_url=${MINIO_ENDPOINT} \ --txlog_rocksdb_cloud_bucket_name="$bucket_name" \ --txlog_rocksdb_cloud_bucket_prefix="$bucket_prefix" \ - --txlog_rocksdb_cloud_object_path="txlog" \ - --txlog_rocksdb_cloud_endpoint_url=${MINIO_ENDPOINT} \ + --txlog_rocksdb_cloud_object_path="txlog" \ + --txlog_rocksdb_cloud_endpoint_url=${MINIO_ENDPOINT} \ --enable_wal=false \concourse/scripts/store_rocksdb_cloud.yaml (1)
1-23: Configuration simplification aligns well with data_substrate migration.The pruned configuration correctly removes legacy eloq, metrics, and cloud storage fields that are now managed through the centralized
data_substrate_configmechanism. The minimal core configuration (ccProtocol, diagnosticDataCollectionEnabled, etc.) supports the streamlined initialization flow.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/mongo/db/modules/eloq/SConscript (2)
36-36: Remove unconditional debug print from FindLibPath.The
print("candidate:",candidate)statement on Line 36 will spam build logs with every library path candidate checked. Guard this with a debug flag or remove it.Apply this fix:
for prefix in prefixes: base_name = prefix + lib_name + ext candidate = os.path.join(path, base_name) - print("candidate:",candidate) + # Uncomment for debugging: print("candidate:",candidate) if os.path.exists(candidate): return candidate
208-208: Inconsistent dependency handling for ROCKSDB.For
DATA_STORE_TYPE_ROCKSDB,store_depsis set to["rocksdb"](a plain string), while other branches useFindLibPath()results. This plain string won't be a valid library path and may cause linking issues.Consider using
FindLibPathconsistently:elif data_store_type == DATA_STORE_TYPE_ROCKSDB: - store_deps = ["rocksdb"] + store_deps = [FindLibPath("rocksdb")]Then filter
Nonevalues when buildingeloq_dependencieson Line 212 as previously suggested.
♻️ Duplicate comments (2)
src/mongo/db/modules/eloq/SConscript (2)
246-252: Debug prints still not guarded (previous issue).The unconditional print statements flagged in the previous review remain unaddressed. These spam build logs and Line 252 uses direct dict access
env['MONGO_ALLOCATOR']which can raise aKeyErrorif the key is not set.Apply the previously suggested fix:
-print("CPPDEFINES: ", env["CPPDEFINES"]) -print("CPPPATH: ", env["CPPPATH"]) -print("LIBPATH: ", env["LIBPATH"]) -print("LIBS: ", env["LIBS"]) -print("LINKFLAGS: ", env["LINKFLAGS"]) -print("eloq_dependencies: ", eloq_dependencies) -print("MALLOC:", env['MONGO_ALLOCATOR']) +if os.environ.get("ELOQ_SCONS_DEBUG"): + print("CPPDEFINES: ", env.get("CPPDEFINES")) + print("CPPPATH: ", env.get("CPPPATH")) + print("LIBPATH: ", env.get("LIBPATH")) + print("LIBS: ", env.get("LIBS")) + print("LINKFLAGS: ", env.get("LINKFLAGS")) + print("eloq_dependencies: ", eloq_dependencies) + print("MALLOC:", env.get('MONGO_ALLOCATOR', '<unset>'))
212-213: Filter None values from dependencies (previous issue).The issue flagged in the previous review remains unaddressed.
FindLibPath(...)can returnNone, and passingNonetoSYSLIBDEPSon Line 283 can break the SCons build.Apply the previously suggested fix:
-eloq_dependencies = base_deps + store_deps +eloq_dependencies = [lib for lib in (base_deps + store_deps) if lib]
🧹 Nitpick comments (1)
src/mongo/db/modules/eloq/SConscript (1)
226-226: Consider guarding this debug print.For consistency with the debug print issue flagged at Lines 246-252, consider guarding this print with a debug environment variable.
Apply this change:
-print("dest_dir: ", dest_dir) +if os.environ.get("ELOQ_SCONS_DEBUG"): + print("dest_dir: ", dest_dir)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mongo/db/modules/eloq/SConscript(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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:
src/mongo/db/modules/eloq/SConscript
📚 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:
src/mongo/db/modules/eloq/SConscript
📚 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:
src/mongo/db/modules/eloq/SConscript
🔇 Additional comments (3)
src/mongo/db/modules/eloq/SConscript (3)
57-58: Define added correctly.The CPPDEFINES entry is now properly formatted without the leading space issue flagged in the previous review.
214-221: Verify that git submodule is initialized and external CMake configuration is correct.The
data_substratedirectory is a git submodule referencinghttps://github.com/eloqdata/tx_service.git, but it is currently uninitialized (empty). The SConscript at lines 214–221 expects Abseil libraries to be built and available in the submodule's build output directory at link time.The library names (
absl_throw_delegate,absl_log_severity,absl_raw_logging_internal,absl_hash,absl_raw_hash_set) follow correct Abseil naming conventions. However, you must:
- Ensure the git submodule is properly initialized and updated
- Verify that the external tx_service CMakeLists.txt actually builds these specific Abseil libraries into the expected output paths
- Confirm the build/data_substrate directory structure will be created with the correct library output
Without access to the external repository's CMake configuration, the availability of these libraries at link time cannot be fully verified from this codebase alone.
232-233: cmake_module_libs are correct; remove debug prints
- Verified: CMake installs targets data_substrate, logservice, txservice, and eloq-metrics (see src/mongo/db/modules/eloq/CMakeLists.txt — add_subdirectory(data_substrate) and install(TARGETS ...)); keeping cmake_module_libs = ["data_substrate","logservice","txservice","eloq-metrics"] is correct.
- Action required: remove the debug print statements added in src/mongo/db/modules/eloq/SConscript (print("dest_dir: ", dest_dir) around line ~226 and the env prints of CPPDEFINES/CPPPATH/LIBPATH/LIBS/LINKFLAGS later); replace with appropriate logging or delete.
⛔ Skipped due to learnings
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.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
concourse/scripts/common.sh (2)
128-132: Fix inconsistent indentation on CPPDEFINES line.Line 132 uses tab indentation while the corresponding line in the standard build (line 79) uses spaces, creating visual inconsistency across the file.
Apply this diff to standardize indentation using spaces:
env OPEN_LOG_SERVICE=0 WITH_DATA_STORE=ELOQDSS_ROCKSDB_CLOUD_S3 WITH_LOG_STATE=ROCKSDB_CLOUD_S3 FORK_HM_PROCESS=1 \ python2 scripts/buildscripts/scons.py MONGO_VERSION=4.0.3 \ VARIANT_DIR=Debug \ CXXFLAGS="-Wno-nonnull -Wno-class-memaccess -Wno-interference-size -Wno-redundant-move" \ - CPPDEFINES="ELOQ_MODULE_ENABLED EXT_TX_PROC_ENABLED" \ + CPPDEFINES="ELOQ_MODULE_ENABLED EXT_TX_PROC_ENABLED" \
157-157: Fix inconsistent indentation on command flags.Multiple command flag lines use tabs while others use spaces, creating inconsistent indentation across both launch_eloqdoc and launch_eloqdoc_fast functions.
Apply this diff to standardize indentation using spaces in launch_eloqdoc:
nohup LD_LIBRARY_PATH=$PREFIX/lib/:${LD_LIBRARY_PATH} $PREFIX/bin/eloqdoc \ --config=./concourse/scripts/store_rocksdb_cloud.yaml \ - --data_substrate_config=./concourse/scripts/data_substrate.cnf \ + --data_substrate_config=./concourse/scripts/data_substrate.cnf \ --rocksdb_cloud_bucket_name="$bucket_name" \ --rocksdb_cloud_bucket_prefix="$bucket_prefix" \ - --rocksdb_cloud_object_path="dss" \ - --rocksdb_cloud_s3_endpoint_url=${MINIO_ENDPOINT} \ + --rocksdb_cloud_object_path="dss" \ + --rocksdb_cloud_s3_endpoint_url=${MINIO_ENDPOINT} \ --txlog_rocksdb_cloud_bucket_name="$bucket_name" \ --txlog_rocksdb_cloud_bucket_prefix="$bucket_prefix" \ - --txlog_rocksdb_cloud_object_path="txlog" \ - --txlog_rocksdb_cloud_endpoint_url=${MINIO_ENDPOINT} \ + --txlog_rocksdb_cloud_object_path="txlog" \ + --txlog_rocksdb_cloud_endpoint_url=${MINIO_ENDPOINT} \ &>$PREFIX/log/eloqdoc.out &Apply the same spacing corrections to launch_eloqdoc_fast (lines 181, 184, 185, 188, 189).
Also applies to: 160-160, 161-161, 164-164, 165-165, 181-181, 184-184, 185-185, 188-188, 189-189
src/mongo/db/modules/eloq/SConscript (6)
100-102: Critical: Include directories still don't exist.These paths reference
data_substrate/log_service/includeanddata_substrate/eloq_log_service/includewhich were verified as non-existent in previous reviews. Thedata_substratedirectory is empty. The build will fail when the compiler cannot locate these include paths.
127-138: Critical: Multiple include paths reference removed submodules.Lines 128-138 reference directories under
data_substrate(tx_service, store_handler, eloq_metrics) that were removed as submodules but never recreated. Previous verification confirmed these paths don't exist:
data_substrate/tx_service/*(lines 128-136)data_substrate/store_handler(line 137)data_substrate/eloq_metrics/include(line 138)The build will fail attempting to locate these include directories.
174-179: Critical: ELOQSTORE include path references non-existent directory.Line 177 references
data_substrate/store_handler/eloq_data_store_service/eloqstore, butstore_handlerwas removed as a submodule and doesn't exist underdata_substrate. This will break builds whenDATA_STORE_TYPE_ELOQSTOREis selected.
212-213: Major: Filter out None values from dependencies.
FindLibPath()returnsNonewhen a library isn't found (line 39), buteloq_dependenciesconcatenatesbase_deps + store_depswithout filtering. PassingNonetoSYSLIBDEPS(line 288) can break the SCons build.Apply this diff:
-eloq_dependencies = base_deps + store_deps +eloq_dependencies = [lib for lib in (base_deps + store_deps) if lib]
214-224: Critical: Abseil library paths depend on non-existent tx_service.Lines 214-224 reference build output under
build/data_substrate/tx_service/abseil-cpp, but the sourcedata_substrate/tx_servicedoesn't exist (flagged in lines 127-138). Without the source, these build artifacts won't be created, causing link failures.
251-258: Major: Prevent KeyError on MONGO_ALLOCATOR access.Line 257 uses direct dictionary access
env['MONGO_ALLOCATOR']which raisesKeyErrorif the key isn't set. Useenv.get()for safety.Apply this diff:
-print("MALLOC:", env['MONGO_ALLOCATOR']) +print("MALLOC:", env.get('MONGO_ALLOCATOR', '<unset>'))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
concourse/scripts/common.sh(6 hunks)src/mongo/db/modules/eloq/SConscript(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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:
src/mongo/db/modules/eloq/SConscript
📚 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:
src/mongo/db/modules/eloq/SConscript
📚 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:
src/mongo/db/modules/eloq/SConscript
🔇 Additional comments (4)
concourse/scripts/common.sh (3)
54-54: Approve cmake BUILD_SHARED_LIBS flag addition.Adding
-DBUILD_SHARED_LIBS=ONto both cmake invocations appropriately enables shared library builds, which aligns with the data substrate migration objectives.Also applies to: 103-103
76-76: Approve scons configuration updates for standard build.The scons invocation correctly enables OPEN_LOG_SERVICE and sets WITH_DATA_STORE to ELOQDSS_ROCKSDB_CLOUD_S3, with proper CPPDEFINES configuration.
Also applies to: 79-79
223-223: Approve LD_LIBRARY_PATH environment updates.Adding
LD_LIBRARY_PATHto the run_jstests environment is appropriate for supporting the shared library builds and aligns with the LD_LIBRARY_PATH additions to the launch functions.src/mongo/db/modules/eloq/SConscript (1)
57-58: LGTM! Public module define added correctly.The
ELOQ_MODULE_ELOQDOCpreprocessor define is now properly formatted without the leading space that was flagged previously.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
concourse/scripts/common.sh (3)
158-166: Fix inconsistent indentation.Several command flag lines use mixed tabs and spaces causing inconsistent indentation. This issue was previously flagged and should be addressed.
172-172: Fix incorrect function name in usage message.The usage message should reference
launch_eloqdoc_fastinstead oflaunch_eloqdoc. This issue was previously flagged.
182-191: Fix inconsistent indentation.Similar to
launch_eloqdoc, several command flag lines use mixed tabs and spaces. This creates the same indentation inconsistency that was previously flagged.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
concourse/scripts/common.sh(6 hunks)concourse/scripts/data_substrate.cnf(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.29.0)
concourse/scripts/data_substrate.cnf
[high] 29-29: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (9)
concourse/scripts/data_substrate.cnf (2)
1-16: Configuration looks appropriate for test environment.The local configuration section properly defines resource limits, feature toggles, and txlog cloud storage settings suitable for a test/development environment.
17-21: LGTM!The cluster configuration appropriately sets up a single-node localhost deployment for testing.
concourse/scripts/common.sh (7)
53-66: LGTM!The cmake build configuration correctly enables shared libraries (
BUILD_SHARED_LIBS=ON) and external transaction processing (EXT_TX_PROC_ENABLED=ON), aligning with the data_substrate migration objectives.
71-89: LGTM!The scons build configuration properly enables the RocksDB Cloud S3 data store and includes the
EXT_TX_PROC_ENABLEDpreprocessor definition, consistent with the cmake configuration.
92-118: LGTM!The enterprise build cmake configuration correctly enables shared libraries and all necessary feature flags for the data_substrate migration.
155-157: Good addition of library path and data substrate configuration.The
LD_LIBRARY_PATHsetup enables the runtime to find the newly built shared libraries, and the--data_substrate_configflag properly wires in the new configuration file.
179-181: Good addition of library path and data substrate configuration.The
LD_LIBRARY_PATHsetup and--data_substrate_configflag are correctly applied, consistent with thelaunch_eloqdocfunction.
221-224: LGTM!The
LD_LIBRARY_PATHaddition ensures that the test runner can locate the shared libraries, consistent with the shared library build configuration.
11-11: No action required—the referenced environment variables are properly set.The MinIO environment variables are correctly defined and exported in the CI entry point scripts (
main.bash,main.ent.bash,pr.ent.bash) beforecommon.shis sourced. The execution order is correct: variables are exported first, thencommon.shis sourced, ensuring they are available when line 11 executes. Externalizing credentials to CI scripts (rather than defining them incommon.sh) is intentional and follows security best practices.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
concourse/scripts/common.sh (2)
157-157: Fix inconsistent indentation (duplicate of prior review).These lines use tab characters for indentation while surrounding lines use spaces, creating visual inconsistency. This issue was flagged in a prior review and remains unresolved.
Apply this diff to standardize indentation using spaces:
nohup env LD_LIBRARY_PATH=$PREFIX/lib/:${LD_LIBRARY_PATH} $PREFIX/bin/eloqdoc \ --config=./concourse/scripts/store_rocksdb_cloud.yaml \ - --data_substrate_config=./concourse/scripts/data_substrate.cnf \ + --data_substrate_config=./concourse/scripts/data_substrate.cnf \ --rocksdb_cloud_bucket_name="$bucket_name" \ --rocksdb_cloud_bucket_prefix="$bucket_prefix" \ - --rocksdb_cloud_object_path="dss" \ - --rocksdb_cloud_s3_endpoint_url=${MINIO_ENDPOINT} \ + --rocksdb_cloud_object_path="dss" \ + --rocksdb_cloud_s3_endpoint_url=${MINIO_ENDPOINT} \ --txlog_rocksdb_cloud_bucket_name="$bucket_name" \ --txlog_rocksdb_cloud_bucket_prefix="$bucket_prefix" \ - --txlog_rocksdb_cloud_object_path="txlog" \ - --txlog_rocksdb_cloud_endpoint_url=${MINIO_ENDPOINT} \ + --txlog_rocksdb_cloud_object_path="txlog" \ + --txlog_rocksdb_cloud_endpoint_url=${MINIO_ENDPOINT} \Also applies to: 160-161, 164-165
172-172: Fix incorrect function name in usage message (duplicate of prior review).The usage message prints
launch_eloqdocbut should printlaunch_eloqdoc_fastsince this is in thelaunch_eloqdoc_fastfunction. This issue was flagged in a prior review and remains unresolved.Apply this diff:
if [ $# -lt 2 ]; then echo "Error: bucket_name and bucket_prefix parameters are required" - echo "Usage: launch_eloqdoc <bucket_name> <bucket_prefix>" + echo "Usage: launch_eloqdoc_fast <bucket_name> <bucket_prefix>" exit 1 fisrc/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (1)
654-656: Apply the same DataSubstrate guards used in createTable.The updateTable function accesses
DataSubstrate::GetGlobal()->GetStoreHandler()without null checks, creating an inconsistency with createTable (lines 519-524) which properly guards these calls. This risks null pointer dereference if DataSubstrate initialization fails or is incomplete.Apply the same defensive pattern used in createTable:
- auto* storeHandler = DataSubstrate::GetGlobal()->GetStoreHandler(); + auto* ds = DataSubstrate::GetGlobal(); + invariant(ds != nullptr); + auto* storeHandler = ds->GetStoreHandler(); + invariant(storeHandler != nullptr); std::string new_kv_info = storeHandler->CreateNewKVCatalogInfo(tableName, currentTableSchema, alterTableInfo); + invariant(!new_kv_info.empty());src/mongo/db/modules/eloq/SConscript (2)
100-102: Verify data_substrate directory structure exists before build.These CPPPATH entries reference subdirectories under
data_substrate/that past analysis indicated don't exist:
data_substrate/log_service/includedata_substrate/eloq_log_service/includedata_substrate/tx_service/include/*data_substrate/store_handlerdata_substrate/eloq_metrics/includeIf these paths are missing, the build will fail when the compiler cannot locate included headers.
Run this script to verify the directory structure:
#!/bin/bash # Verify data_substrate subdirectories exist base="src/mongo/db/modules/eloq/data_substrate" echo "Checking data_substrate structure..." for dir in \ "log_service/include" \ "eloq_log_service/include" \ "tx_service/include" \ "tx_service/include/cc" \ "tx_service/include/sequences" \ "store_handler" \ "eloq_metrics/include"; do if [ -d "$base/$dir" ]; then echo "✓ $dir" else echo "✗ $dir MISSING" fi doneAlso applies to: 127-138, 177-177
251-257: Use env.get() to avoid KeyError in debug prints.Direct dictionary access with
env['MONGO_ALLOCATOR']will raiseKeyErrorif the key is undefined. The other print statements are safe but verbose.Apply this diff for safer access:
print("CPPDEFINES: ", env["CPPDEFINES"]) print("CPPPATH: ", env["CPPPATH"]) print("LIBPATH: ", env["LIBPATH"]) print("LIBS: ", env["LIBS"]) print("LINKFLAGS: ", env["LINKFLAGS"]) print("eloq_dependencies: ", eloq_dependencies) -print("MALLOC:", env['MONGO_ALLOCATOR']) +print("MALLOC:", env.get('MONGO_ALLOCATOR', '<unset>'))If these debug prints are no longer needed after development, consider removing them to reduce log verbosity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
concourse/scripts/common.sh(6 hunks)src/mongo/db/modules/eloq/SConscript(6 hunks)src/mongo/db/modules/eloq/src/base/eloq_table_schema.cpp(3 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(7 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.h(3 hunks)src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp(3 hunks)src/mongo/db/server_options_server_helpers.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mongo/db/modules/eloq/src/base/eloq_table_schema.cpp
🧰 Additional context used
🧠 Learnings (4)
📚 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:
src/mongo/db/modules/eloq/src/eloq_kv_engine.cppsrc/mongo/db/server_options_server_helpers.cpp
📚 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:
src/mongo/db/modules/eloq/src/eloq_kv_engine.cppsrc/mongo/db/modules/eloq/SConscript
📚 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:
src/mongo/db/modules/eloq/src/eloq_kv_engine.cppsrc/mongo/db/modules/eloq/SConscript
📚 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:
src/mongo/db/modules/eloq/SConscript
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/eloq_kv_engine.h (1)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (2)
EloqKVEngine(163-176)EloqKVEngine(178-183)
🔇 Additional comments (20)
concourse/scripts/common.sh (5)
54-54: Build system correctly updated with shared library flag.The addition of
-DBUILD_SHARED_LIBS=ONaligns with the Data Substrate migration and enables shared library builds as intended.Also applies to: 103-103
76-76: Environment and build configuration updates are appropriate.The environment variables (
OPEN_LOG_SERVICE,WITH_DATA_STORE) and CPPDEFINES flags correctly reflect the new data_substrate-aware build configuration.Also applies to: 128-132
155-156: Runtime launcher correctly updated with LD_LIBRARY_PATH and new config flags.The addition of
LD_LIBRARY_PATHprefix to eloqdoc invocation and the new CLI flags (--config, --data_substrate_config, --rocksdb_cloud_*) correctly support the Data Substrate migration.Also applies to: 158-159, 162-163, 166-166
223-223: Environment variables correctly added to run_jstests.The addition of
LD_LIBRARY_PATHandPATHenvironment variables ensures proper runtime library discovery for the test suite, aligning with the shared library build changes.
188-188: Parameter name mismatch—fix missingtxlog_prefix.Line 188 uses
--rocksdb_cloud_object_path="txlog"but this should be--txlog_rocksdb_cloud_object_path="txlog"to match the naming convention used inlaunch_eloqdoc(line 164) and maintain consistency with other txlog parameters on lines 186–187 and 189.Apply this diff:
--txlog_rocksdb_cloud_bucket_name="$bucket_name" \ --txlog_rocksdb_cloud_bucket_prefix="$bucket_prefix" \ - --rocksdb_cloud_object_path="txlog" \ + --txlog_rocksdb_cloud_object_path="txlog" \ --txlog_rocksdb_cloud_endpoint_url=${MINIO_ENDPOINT} \Likely an incorrect or invalid review comment.
src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (2)
35-47: LGTM: Include paths successfully migrated to data_substrate structure.The header includes have been properly updated to reference the new data_substrate module paths, aligning with the PR's objective of converging support around the data_substrate architecture.
519-524: LGTM: Proper defensive guards implemented.The null pointer guards and validation checks correctly prevent potential crashes from uninitialized DataSubstrate or StoreHandler. The invariant checks provide clear failure points for debugging.
src/mongo/db/modules/eloq/src/eloq_kv_engine.h (3)
32-39: LGTM: Include paths migrated to data_substrate.Header includes properly updated to reference log_service and tx_service from the data_substrate module structure.
66-69: LGTM: Explicit copy/move semantics prevent lifetime bugs.Deleting the copy and move operations makes the non-owning pointer semantics explicit and prevents accidental copying of the EloqKVEngine with its raw pointer members.
200-201: LGTM: Raw pointers properly initialized to nullptr.The non-owning raw pointers are now default-initialized to nullptr, preventing undefined behavior from uninitialized pointer access and making the ownership model explicit through documentation comments.
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (6)
37-60: LGTM: Include paths successfully migrated to data_substrate.The header includes have been comprehensively updated to reference data_substrate module paths for metrics, store_handler, and tx_service components.
126-131: LGTM: Global registration enables data_substrate linkage.The catalog factory and system handler are properly registered via global pointers, allowing data_substrate to link against these implementations without circular dependencies.
133-134: LGTM: Flag naming avoids MongoDB options_parser conflicts.Using
--data_substrate_configwith a distinct prefix avoids conflicts with MongoDB's options_parser ownership of--config, consistent with repository conventions.Based on learnings
165-169: LGTM: DataSubstrate initialization properly sequenced.The standalone mode initialization sequence correctly initializes DataSubstrate before retrieving service pointers, with appropriate logging for diagnostics.
196-197: LGTM: Defensive guard prevents invalid RecoveryUnit construction.The invariant check ensures
_txServiceis valid before constructing EloqRecoveryUnit, preventing crashes from uninitialized service pointers.
583-588: LGTM: Safe shutdown with null guard.The null check on
data_substratebefore callingShutdown()prevents crashes during cleanup if DataSubstrate was never initialized or already torn down.src/mongo/db/modules/eloq/SConscript (3)
57-57: LGTM: Module identifier define added.The
ELOQ_MODULE_ELOQDOCpreprocessor define properly identifies this build variant without the leading space issue from earlier versions.
212-212: LGTM: Dependencies properly filtered.The list comprehension correctly filters out None values returned by
FindLibPath(), preventing SCons errors from invalid library paths.
214-224: LGTM: Abseil library setup properly structured.The absl library path configuration correctly prepends search paths and links required absl components for the data_substrate tx_service dependency.
src/mongo/db/server_options_server_helpers.cpp (1)
133-137: Option registration looks good, but ensure runtime validation is correct.The
net.adaptiveThreadNumoption is properly registered with a sensible default. The validation logic instoreServerOptionsshould ensure this is only used whenserviceExecutoris set to "adaptive".
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
concourse/artifact/ELOQDSS_ROCKSDB/data_substrate.cnf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate.cnf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate_cluster_a.cnf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate_cluster_b.cnf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate_cluster_c.cnf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_a.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_b.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_c.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdss.conf(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate_cluster_b.cnfconcourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate.cnf
📚 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/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.confconcourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdss.confconcourse/artifact/ELOQDSS_ROCKSDB/eloqdoc.conf
🪛 Gitleaks (8.29.0)
concourse/artifact/ELOQDSS_ROCKSDB/data_substrate.cnf
[high] 29-29: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (6)
concourse/artifact/ELOQDSS_ROCKSDB/eloqdoc.conf (1)
12-24: Net-level threads + txService.ccProtocol change look consistentMoving
adaptiveThreadNumundernetand reducingeloq.txServiceto onlyccProtocol: "OccRead"matches the converged pattern used elsewhere, and the addedttlMonitorEnabled: falseis a common test/CI setting. I don’t see issues with these values as written.concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdss.conf (1)
7-14: Newrocksdb_cloud_object_pathaligns with existing txlog object pathsAdding
rocksdb_cloud_object_path=eloqdoc_dssunder[store]cleanly mirrors the txlog object path settings in the data_substrate configs (separate paths for txlog vs DSS store). The wiring with the local DSS endpoint at 127.0.0.1:9100 looks consistent.concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_b.conf (1)
12-24: Cluster‑B net/eloq/setParameter changes are consistent with the converged configShifting thread settings to
net.adaptiveThreadNum, simplifyingeloq.txServicetoccProtocol: "OccRead", and adding the two setParameter flags matches the pattern used in the non‑cloud and other cluster configs. The cluster‑B–specific ports and paths look coherent.concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf (1)
12-19: net adaptiveThreadNum + ccProtocol config looks consistentPlumbing
adaptiveThreadNum: 2undernetalongsideserviceExecutor: "adaptive"and keepingtxService.ccProtocol: "OccRead"matches the converged pattern in the other eloqdoc cluster configs; this looks coherent for the new data-substrate-driven setup.concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_a.conf (1)
12-19: Cluster A adaptiveThreadNum/ccProtocol settings look goodThe
net.adaptiveThreadNum: 2andtxService.ccProtocol: "OccRead"changes are consistent with the non-cluster and other cluster configs and fit the new converged layout.concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_c.conf (1)
12-19: Cluster C adaptiveThreadNum/ccProtocol config is consistent
adaptiveThreadNum: 2undernetandccProtocol: "OccRead"undertxServicealign with the other eloqdoc configs in this PR; nothing problematic stands out here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (3)
196-198: Good safeguard: ensure_txServiceis non‑null before constructing RUAdding
invariant(_txService != nullptr);before constructingEloqRecoveryUnitcloses the gap where a misconfigured engine could have produced a RU with a null service pointer. This matches the earlier recommendation and should make failures much easier to diagnose.
592-595: Clean shutdown now guards DataSubstrate and clears pointersThe new pattern in
cleanShutdown()that fetchesDataSubstrate::GetGlobal()into a local, checks for nullptr before callingShutdown(), and then nulls out_txServiceand_logServeravoids potential null dereferences and double-shutdown issues while preserving the existing coroutine/thread synchronization pattern.Also applies to: 603-604
165-176: Validate DataSubstrate initialization before dereferencing GetGlobalRight now the ctor calls
DataSubstrate::InitializeGlobal(FLAGS_data_substrate_config);and then unconditionally dereferencesDataSubstrate::GetGlobal()twice to pull the log server and tx service. If initialization can fail orGetGlobal()can returnnullptr, this will crash here with a null dereference and only later do we have the_txServiceinvariant innewRecoveryUnit().Reintroducing a local
DataSubstrate*with explicit validation and clear logging/uassert would make this much more robust and fail-fast, as previously discussed.EloqKVEngine::EloqKVEngine(const std::string& path) : _dbPath(path) { log() << "Starting Eloq storage engine. dbPath: " << path; log() << "Standalone mode: Initializing data substrate..."; - DataSubstrate::InitializeGlobal(FLAGS_data_substrate_config); - - _logServer = DataSubstrate::GetGlobal()->GetLogServer(); - _txService = DataSubstrate::GetGlobal()->GetTxService(); + DataSubstrate::InitializeGlobal(FLAGS_data_substrate_config); + auto* ds = DataSubstrate::GetGlobal(); + invariant(ds != nullptr); + _logServer = ds->GetLogServer(); + _txService = ds->GetTxService(); + invariant(_txService != nullptr); + invariant(_logServer != nullptr); @@ log() << "Eloq storage engine initialized."; }If
InitializeGlobalalready throws or aborts on failure in your build, you can tune the validation accordingly, but it’s still worth avoiding rawGetGlobal()dereferences.
🧹 Nitpick comments (2)
src/mongo/db/modules/eloq/src/eloq_kv_engine.h (1)
198-199: Raw members correctly default‑initialized; consider clarifying non‑ownershipDefault‑initializing
_txServiceand_logServertonullptris the right move to avoid accidental use of indeterminate pointers. If it’s not documented elsewhere, a brief comment noting that these are non‑owning views into Data Substrate state would make the intent clearer for future maintainers.src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1)
126-132: Static catalog/system handler wiring works but depends on init orderingRegistering a
MongoCatalogFactoryandMongoSystemHandleras global objects with exported pointers (eloqdoc_catalog_factory,eloqdoc_system_handler) is a reasonable way to integrate with Data Substrate, but it does lean on static-initialization ordering between this TU and any TU that reads those externs. If you ever see subtle startup-time issues, consider switching to an explicit registration API that’s called fromEloqKVEngine’s constructor instead of static globals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(7 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.h(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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:
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
📚 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:
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
📚 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:
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
📚 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:
src/mongo/db/modules/eloq/src/eloq_kv_engine.h
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/eloq_kv_engine.h (1)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (2)
EloqKVEngine(163-176)EloqKVEngine(178-183)
🔇 Additional comments (7)
src/mongo/db/modules/eloq/src/eloq_kv_engine.h (2)
31-39: Include-path migration to Data Substrate log/tx service looks consistentThe updated log_server/log_utils and tx_service/sharder includes under
data_substratealign with the new architecture and match the cpp usage; no issues spotted here.
66-69: Good: make raw‑pointer‑bearing engine non‑copyable/non‑movableDeleting copy and move special members for
EloqKVEngineprevents accidental copying/moving of an object that only holds non‑owning pointers, which would be a lifetime footgun otherwise.src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (5)
37-42: Data Substrate metrics/kv/tx header migration looks fineThe new
data_substrateincludes for metrics, kv_store, and tx_service catalog/deadlock/sequences types are consistent with the refactor and with the rest of this TU; they should preserve behavior as long as the new headers expose the same interfaces.
53-60: Core tx_service/Data Substrate includes are wired coherentlyBringing in
tx_key,tx_record,tx_service,tx_service_metrics,tx_util,type, anddata_substrate.hfrom thedata_substratetree matches the new dependency direction and supports the laterDataSubstrate::InitializeGlobalusage; nothing problematic here.
78-81: Store handler includes moved under data_substrate namespaceThe store handler client and Eloq data store service/config headers under
mongo/db/modules/eloq/data_substrate/store_handler/...look like a straightforward path tightening while preserving the existing conditional compilation onELOQDS. This is consistent with the converged data_substrate layout.
123-124: RocksDB cloud config include path matches LOG_STATE macro contractPulling
rocksdb_cloud_config.hfrom thedata_substrate/eloq_log_servicetree underLOG_STATE_TYPE_RKDB_CLOUDkeeps the cloud-specific config isolated to S3/GCS builds and does not affect plain RKDB builds, matching the earlier “RKDB_CLOUD only for cloud variants” constraint. Based on learnings.
133-137: No issues found; gflags definition is correct.Verification confirms exactly one
DEFINE_string(data_substrate_config, ...)exists at eloq_kv_engine.cpp:134, with a correspondingDECLARE_stringin eloq_global_options.cpp:36 for cross-file use. This is the correct gflags pattern and avoids ODR/link issues. The distinct prefix approach aligns with repository guidance.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
concourse/scripts/common.sh (2)
188-199: ⚠ Indentation inconsistency persists (duplicate concern from previous review).Lines 190, 193, 197–198 continue to use mixed tabs and spaces despite being flagged as addressed in previous commits. This causes visual inconsistency and can break in some contexts.
Ensure all indentation uses spaces consistently:
nohup env LD_LIBRARY_PATH=$PREFIX/lib/:${LD_LIBRARY_PATH} $PREFIX/bin/eloqdoc \ --config=./concourse/scripts/store_rocksdb_cloud.yaml \ - --data_substrate_config=./concourse/scripts/data_substrate.cnf \ + --data_substrate_config=./concourse/scripts/data_substrate.cnf \ --rocksdb_cloud_bucket_name="$bucket_name" \ --rocksdb_cloud_bucket_prefix="$bucket_prefix" \ - --rocksdb_cloud_object_path="dss" \ - --rocksdb_cloud_s3_endpoint_url=${ELOQ_AWS_S3_ENDPOINT_URL} \ + --rocksdb_cloud_object_path="dss" \ + --rocksdb_cloud_s3_endpoint_url=${ELOQ_AWS_S3_ENDPOINT_URL} \ --txlog_rocksdb_cloud_bucket_name="$bucket_name" \ --txlog_rocksdb_cloud_bucket_prefix="$bucket_prefix" \ - --txlog_rocksdb_cloud_object_path="txlog" \ - --txlog_rocksdb_cloud_endpoint_url=${ELOQ_AWS_S3_ENDPOINT_URL} \ + --txlog_rocksdb_cloud_object_path="txlog" \ + --txlog_rocksdb_cloud_endpoint_url=${ELOQ_AWS_S3_ENDPOINT_URL} \ &>$PREFIX/log/eloqdoc.out &
202-225: Fix incorrect usage message (duplicate concern from previous review).Line 205 incorrectly prints
launch_eloqdocin the error message but should printlaunch_eloqdoc_fastto match the actual function name. Additionally, lines 214, 217, 221–222 retain the mixed tab/space indentation from the parent function.Apply this diff:
if [ $# -lt 2 ]; then echo "Error: bucket_name and bucket_prefix parameters are required" - echo "Usage: launch_eloqdoc <bucket_name> <bucket_prefix>" + echo "Usage: launch_eloqdoc_fast <bucket_name> <bucket_prefix>" exit 1 fi local bucket_name="$1" local bucket_prefix="$2" echo "launch eloqdoc with bucket name: $bucket_name, bucket prefix: $bucket_prefix" mkdir -p "$PREFIX/log" "$PREFIX/data" nohup env LD_LIBRARY_PATH=$PREFIX/lib/:${LD_LIBRARY_PATH} $PREFIX/bin/eloqdoc \ --config=./concourse/scripts/store_rocksdb_cloud.yaml \ - --data_substrate_config=./concourse/scripts/data_substrate.cnf \ + --data_substrate_config=./concourse/scripts/data_substrate.cnf \ --rocksdb_cloud_bucket_name="$bucket_name" \ --rocksdb_cloud_bucket_prefix="$bucket_prefix" \ - --rocksdb_cloud_object_path="dss" \ - --rocksdb_cloud_s3_endpoint_url=${ELOQ_AWS_S3_ENDPOINT_URL} \ + --rocksdb_cloud_object_path="dss" \ + --rocksdb_cloud_s3_endpoint_url=${ELOQ_AWS_S3_ENDPOINT_URL} \ --txlog_rocksdb_cloud_bucket_name="$bucket_name" \ --txlog_rocksdb_cloud_bucket_prefix="$bucket_prefix" \ - --txlog_rocksdb_cloud_object_path="txlog" \ - --txlog_rocksdb_cloud_endpoint_url=${ELOQ_AWS_S3_ENDPOINT_URL} \ + --txlog_rocksdb_cloud_object_path="txlog" \ + --txlog_rocksdb_cloud_endpoint_url=${ELOQ_AWS_S3_ENDPOINT_URL} \ --enable_wal=false \ &>$PREFIX/log/eloqdoc.out &
🧹 Nitpick comments (2)
concourse/scripts/pr.ent.bash (1)
4-4: Consider parameterizing the PREFIX path via environment variables.Line 4 hardcodes an absolute path that may vary across environments. While the past review flagged credentials (which appear to have been removed), this configuration path could benefit from the same environment-variable pattern to increase portability.
Apply this diff to make PREFIX configurable with a default fallback:
-export PREFIX="/home/eloq/workspace/mongo/install" +export PREFIX="${PREFIX:=/home/eloq/workspace/mongo/install}"This allows PREFIX to be overridden by environment while maintaining a sensible default. Alternatively, if PREFIX is workspace-specific and should never change, you may leave it as-is.
concourse/scripts/common.sh (1)
36-44: Missing error handling for update_config_template calls.The function returns an error status (line 18) when a config file doesn't exist, but the calling code on lines 36-44 doesn't check the return value. If a required config file is missing, the script will continue without warning.
Apply this diff to halt on missing config files:
update_config_template ./data_substrate.cnf + update_config_template ./artifact/ELOQDSS_ROCKSDB/data_substrate.cnf + update_config_template ./artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate.cnfOr, improve the function to abort on first missing file:
update_config_template() { local config_file="$1" if [ ! -f "$config_file" ]; then echo "Warning: Config file $config_file does not exist" - return 1 + echo "Error: Required config file missing: $config_file" + exit 1 fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
concourse/artifact/ELOQDSS_ROCKSDB/data_substrate.cnf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate_cluster_a.cnf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate_cluster_b.cnf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate_cluster_c.cnf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdss.conf(1 hunks)concourse/pipeline/main.ent.yml(1 hunks)concourse/pipeline/main.yml(1 hunks)concourse/pipeline/pr.ent.yml(1 hunks)concourse/scripts/common.sh(7 hunks)concourse/scripts/data_substrate.cnf(1 hunks)concourse/scripts/main.bash(1 hunks)concourse/scripts/main.ent.bash(1 hunks)concourse/scripts/pr.ent.bash(3 hunks)src/mongo/db/modules/eloq/data_substrate(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/mongo/db/modules/eloq/data_substrate
- concourse/scripts/data_substrate.cnf
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate_cluster_b.cnf
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdss.conf
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate_cluster_c.cnf
- concourse/scripts/main.bash
🧰 Additional context used
🧠 Learnings (1)
📚 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/artifact/ELOQDSS_ROCKSDB/data_substrate.cnfconcourse/scripts/common.sh
🔇 Additional comments (13)
concourse/scripts/pr.ent.bash (1)
66-69: Timestamped bucket naming improves parallel test isolation.The introduction of millisecond-precision timestamps (line 66) for bucket naming is a solid improvement that prevents collisions across parallel test runs. The echo on line 69 provides good observability for debugging and log analysis.
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate_cluster_a.cnf (1)
20-23: Previous critical issue has been resolved.The delimiter typo in
ip_port_listflagged in the previous review has been fixed. The value now correctly uses commas to separate all threeip:portpairs:127.0.0.1:9200,127.0.0.1:9210,127.0.0.1:9220.Additionally, the configuration is internally consistent:
node_group_replica_num=3matches the three entries inip_port_list- The first entry
127.0.0.1:9200matchestx_ipandtx_portfrom the[local]sectionconcourse/scripts/main.ent.bash (1)
1-57: LGTM — environment setup properly delegated to sourced script.The file correctly exports workspace/infrastructure variables and delegates credential/config setup to
common.sh. The past security concerns about hardcoded MinIO credentials appear to have been remediated by externalizing them to environment variables.concourse/pipeline/pr.ent.yml (1)
69-72: ✓ Vault credential injection properly implemented.The ELOQ_AWS_* environment variables are correctly sourced from Concourse vault placeholders, avoiding exposure of secrets in the pipeline definition. Ensure the corresponding vault secrets (
minio-access-key,aws-region,minio-secret-key,minio-endpoint) are configured in your Concourse credential manager before deployment.concourse/pipeline/main.ent.yml (1)
54-58: ✓ Consistent vault credential pattern across pipelines.The AWS environment parameters align with the PR pipeline configuration and properly reference vault placeholders rather than hardcoding secrets.
concourse/pipeline/main.yml (1)
48-52: ✓ Credential handling consistent across all pipeline variants.The main pipeline correctly mirrors the AWS environment parameter pattern from the ENT variants, using vault placeholders for secrets management.
concourse/scripts/common.sh (5)
11-11: ✓ Credentials properly sourced from environment variables.The MinIO setup correctly uses
${ELOQ_AWS_*}environment variables instead of hardcoded secrets, addressing the security concern from the previous review.
14-34: Verify sed pattern change on line 26 — possible key name typo.Line 26 changes the config key from
txlog_rocksdb_cloud_s3_endpoint_urltoeloq_txlog_rocksdb_cloud_s3_endpoint_url. This key rename may be intentional for consolidation, but please confirm it matches expected config schema.Additionally, multiple sed substitutions without intermediate validation create risk of silent failures if patterns don't match. Consider validating that critical keys were updated.
76-123: ✓ Build configuration properly aligned with data-substrate consolidation.The cmake and scons flags correctly introduce shared library support and explicitly set the data store variant (
ELOQDSS_ROCKSDB_CLOUD_S3), consistent with the PR's consolidation goals.
125-176: ✓ ENT build variant properly configured with data-substrate flags.The enterprise build configuration correctly parallels the standard build with appropriate ENT-specific flags (log state, fork HM process) while maintaining consistency in data store selection.
227-273: ✓ Test and helper functions properly configured with library paths.The helper functions (
shutdown_eloqdoc,try_connect) and test runners (run_jstests,run_tpcc) correctly set environment paths and implement appropriate retry logic. The 30-second connection retry with 1-second intervals is reasonable for integration testing.concourse/artifact/ELOQDSS_ROCKSDB/data_substrate.cnf (2)
28-29: Good use of placeholder credentials.Unlike the ELOQDSS_ROCKSDB_CLOUD_S3 variant flagged in previous reviews, this configuration correctly uses placeholder values (
CHANGEME_*) instead of hard-coded credentials. This is the appropriate approach for version-controlled configuration files.Ensure deployment documentation clearly specifies that these placeholders must be replaced with actual credentials via environment variables, CI secrets, or a secrets manager.
12-15: Verify whether txlog and store share authentication or use separate mechanisms.The consistent absence of
aws_access_key_idandaws_secret_keyin the[local]section's txlog cloud configuration (lines 12-15) is replicated across all similar configuration files in the repository, including the ELOQDSS_ROCKSDB_CLOUD_S3 variant. This pattern suggests the design may be intentional—txlog cloud access may be authenticated through:
- A shared or inherited mechanism from the store section
- Environment variables or IAM roles at runtime
- A different authentication path not specified in the config file
The store section correctly uses placeholder credentials (
CHANGEME_*), addressing credential exposure concerns.To proceed, manually verify:
- Application documentation or source code explaining txlog cloud authentication
- Whether credentials should match the store section for consistency
- If current design is intentional or an oversight
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
concourse/scripts/common.sh (1)
207-207: Fix incorrect function name in usage message.The usage message in
launch_eloqdoc_fastprintslaunch_eloqdocbut should printlaunch_eloqdoc_fast, causing user confusion when the function fails validation.if [ $# -lt 2 ]; then echo "Error: bucket_name and bucket_prefix parameters are required" - echo "Usage: launch_eloqdoc <bucket_name> <bucket_prefix>" + echo "Usage: launch_eloqdoc_fast <bucket_name> <bucket_prefix>" exit 1 fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
concourse/scripts/common.sh(7 hunks)
🔇 Additional comments (1)
concourse/scripts/common.sh (1)
256-260: Correct use of LD_LIBRARY_PATH for runtime linking.The addition of
LD_LIBRARY_PATHto the environment ensures proper resolution of shared libraries built with-DBUILD_SHARED_LIBS=ON. This is consistent with the dynamic linking approach used in launch functions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
concourse/scripts/common.sh (3)
26-26: Fix config key name inconsistency in sed substitutions (still present from previous review).Lines 26 and 32 search for keys without the
eloq_prefix but replace with keys that include it, causing the sed patterns to rename config keys instead of updating values. This leaves the original config keys unchanged.- sed -i "s|txlog_rocksdb_cloud_s3_endpoint_url.*=.\+|eloq_txlog_rocksdb_cloud_s3_endpoint_url=${ELOQ_AWS_S3_ENDPOINT_URL}|g" "$config_file" + sed -i "s|txlog_rocksdb_cloud_s3_endpoint_url.*=.\+|txlog_rocksdb_cloud_s3_endpoint_url=${ELOQ_AWS_S3_ENDPOINT_URL}|g" "$config_file"- sed -i "s/txlog_rocksdb_cloud_bucket_prefix.*=.\+/eloq_txlog_rocksdb_cloud_bucket_prefix=${BUCKET_PREFIX}/g" "$config_file" + sed -i "s/txlog_rocksdb_cloud_bucket_prefix.*=.\+/txlog_rocksdb_cloud_bucket_prefix=${BUCKET_PREFIX}/g" "$config_file"Also applies to: 32-32
192-192: Fix inconsistent indentation inlaunch_eloqdoc()command flags (still present from previous review).Lines 192, 195, 196, 199, and 200 use tab characters for indentation while adjacent lines use spaces, creating visual inconsistency with the properly-formatted
launch_eloqdoc_fast()function below.nohup env LD_LIBRARY_PATH=$PREFIX/lib/:${LD_LIBRARY_PATH} $PREFIX/bin/eloqdoc \ --config=./concourse/scripts/store_rocksdb_cloud.yaml \ - --data_substrate_config=./concourse/scripts/data_substrate.cnf \ + --data_substrate_config=./concourse/scripts/data_substrate.cnf \ --rocksdb_cloud_bucket_name="$bucket_name" \ --rocksdb_cloud_bucket_prefix="$bucket_prefix" \ - --rocksdb_cloud_object_path="dss" \ - --rocksdb_cloud_s3_endpoint_url=${ELOQ_AWS_S3_ENDPOINT_URL} \ + --rocksdb_cloud_object_path="dss" \ + --rocksdb_cloud_s3_endpoint_url=${ELOQ_AWS_S3_ENDPOINT_URL} \ --txlog_rocksdb_cloud_bucket_name="$bucket_name" \ --txlog_rocksdb_cloud_bucket_prefix="$bucket_prefix" \ - --txlog_rocksdb_cloud_object_path="txlog" \ - --txlog_rocksdb_cloud_s3_endpoint_url=${ELOQ_AWS_S3_ENDPOINT_URL} \ + --txlog_rocksdb_cloud_object_path="txlog" \ + --txlog_rocksdb_cloud_s3_endpoint_url=${ELOQ_AWS_S3_ENDPOINT_URL} \ &>$PREFIX/log/eloqdoc.out &Also applies to: 195-200
207-207: Fix incorrect function name in usage message (still present from previous review).The usage message in
launch_eloqdoc_fast()printslaunch_eloqdocbut should printlaunch_eloqdoc_fast, causing user confusion when the function fails validation.if [ $# -lt 2 ]; then echo "Error: bucket_name and bucket_prefix parameters are required" - echo "Usage: launch_eloqdoc <bucket_name> <bucket_prefix>" + echo "Usage: launch_eloqdoc_fast <bucket_name> <bucket_prefix>" exit 1 fi
🧹 Nitpick comments (1)
concourse/scripts/main.ent.bash (1)
4-7: Consolidation of environment variable exports looks good, but PREFIX could be more flexible.Lines 4–5 properly export
WORKSPACEandCASS_HOSTat the script's top level, which improves visibility and allows easier override. However, line 7 hardcodes thePREFIXpath, which reduces flexibility across different environments.Consider using an environment variable pattern with a default fallback to maintain backward compatibility while allowing customization:
-export PREFIX="/home/eloq/workspace/mongo/install" +export PREFIX="${PREFIX:=/home/eloq/workspace/mongo/install}"This allows operators to override
PREFIXfrom the CI/CD environment while maintaining the default if not set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
concourse/scripts/common.sh(7 hunks)concourse/scripts/main.ent.bash(3 hunks)
🔇 Additional comments (3)
concourse/scripts/main.ent.bash (3)
34-39: Data substrate submodule structure update looks correct.The change to
cd src/mongo/db/modules/eloq/data_substrate(line 34) and the subsequent symbolic link creations align well with the PR's objective of consolidating Eloq components under the data_substrate submodule. This structure reorganization appears sound.
44-47: Timestamp-based bucket naming for test isolation is a solid pattern.Lines 44–46 implement timestamp-based uniqueness for bucket names, which is a robust approach to test isolation and prevents naming conflicts across concurrent or sequential test runs. The calculation using nanosecond precision is appropriate for this use case.
1-60: Verification passed: Hardcoded credentials issue has been resolved.The refactored code properly addresses the critical security vulnerability flagged in the previous review. The hardcoded MinIO credentials have been removed and replaced with environment variable injection. The
common.shfile correctly uses injected credentials (ELOQ_AWS_ACCESS_KEY_ID, ELOQ_AWS_SECRET_KEY, ELOQ_AWS_S3_ENDPOINT_URL, ELOQ_AWS_REGION) rather than hardcoding them, following security best practices. No credential-related issues found.
Reorg include after include data_substrate Access storeHandler from DataSubstrate::Global() Reorg members of EloqKVEngine Reorg build scripts Add gflags in dbmain.cpp Remove submodules of eloq_metrics, store_handler, log_service tx_service submodule Add data_substrate submodule Remove eloq paramters from options parser Ignore unknow options errors when given eloq config from command line Move adaptiveThreadNum to net Read and reuse core_num and bootstap flags from data_substrate config Update concourse scripts Add LD_LIBRARY_PATH for smoke test Update concourse config Fix shutdown when eloq module disabled
githubzilla
force-pushed
the
converge_support_eloqdoc
branch
from
090b9c1 to
06b3693
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mongo/db/modules/eloq/src/eloq_global_options.h (1)
23-27: Remove duplicate includes.Lines 23-24 and 26-27 contain duplicate
#includedirectives for<cstdint>and<sys/types.h>.#include "mongo/util/options_parser/startup_options.h" #include <cstdint> #include <sys/types.h> - -#include <cstdint> -#include <sys/types.h> #include "mongo/db/modules/eloq/data_substrate/tx_service/include/cc_protocol.h"
♻️ Duplicate comments (8)
.gitignore (1)
186-188: Scope bare patterns to prevent unintended ignores.The bare patterns
data,pyvenv, andvenvwill ignore any file or directory with these names anywhere in the repository tree. Whilepyvenvandvenvare typically top-level Python virtual environments, they can appear in nested source paths. Thedatapattern is especially risky, as it can silently hide source directories likesrc/.../data.For consistency and safety, scope these to the top-level only:
Apply this diff:
-data -pyvenv -venv +/data +/pyvenv +/venv(The leading
/anchors these patterns to the repository root, matching only top-level directories.)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate.cnf (1)
22-29: Plaintext AWS-style credentials in tracked artifact (must be removed/rotated)Lines 28–29 contain what look like real AWS-style access key and secret in a committed config file. This is a high‑severity credential leak: treat these values as compromised, rotate/revoke them outside the repo, and ensure they are never reintroduced in plaintext.
Instead, parameterize them and inject from Concourse/secret management (Vault/CredHub/env), e.g.:
-aws_access_key_id=35cxOCh64Ef1Mk5U1bgU -aws_secret_key=M6oJQWdFCr27TUUS40wS6POQzbKhbFTHG9bRayoC +aws_access_key_id=((rocksdb_cloud_aws_access_key_id)) +aws_secret_key=((rocksdb_cloud_aws_secret_key))and wire
rocksdb_cloud_aws_access_key_id/rocksdb_cloud_aws_secret_keyvia your pipeline/secret store rather than committing them here.concourse/scripts/common.sh (2)
190-201: Fix inconsistent indentation in launch_eloqdoc() command.Lines 192, 195, 196, 199, and 200 use tab characters for indentation while surrounding lines use spaces. This creates visual inconsistency and should be standardized to spaces.
Apply this diff to standardize indentation to spaces:
nohup env LD_LIBRARY_PATH=$PREFIX/lib/:${LD_LIBRARY_PATH} $PREFIX/bin/eloqdoc \ --config=./concourse/scripts/store_rocksdb_cloud.yaml \ - --data_substrate_config=./concourse/scripts/data_substrate.cnf \ + --data_substrate_config=./concourse/scripts/data_substrate.cnf \ --rocksdb_cloud_bucket_name="$bucket_name" \ --rocksdb_cloud_bucket_prefix="$bucket_prefix" \ - --rocksdb_cloud_object_path="dss" \ - --rocksdb_cloud_s3_endpoint_url=${ELOQ_AWS_S3_ENDPOINT_URL} \ + --rocksdb_cloud_object_path="dss" \ + --rocksdb_cloud_s3_endpoint_url=${ELOQ_AWS_S3_ENDPOINT_URL} \ --txlog_rocksdb_cloud_bucket_name="$bucket_name" \ --txlog_rocksdb_cloud_bucket_prefix="$bucket_prefix" \ - --txlog_rocksdb_cloud_object_path="txlog" \ - --txlog_rocksdb_cloud_s3_endpoint_url=${ELOQ_AWS_S3_ENDPOINT_URL} \ + --txlog_rocksdb_cloud_object_path="txlog" \ + --txlog_rocksdb_cloud_s3_endpoint_url=${ELOQ_AWS_S3_ENDPOINT_URL} \ &>$PREFIX/log/eloqdoc.out &
207-207: Fix incorrect function name in usage message.The usage message on line 207 prints "launch_eloqdoc" but should print "launch_eloqdoc_fast" to match the actual function name. This causes user confusion when validation fails.
Apply this diff:
if [ $# -lt 2 ]; then echo "Error: bucket_name and bucket_prefix parameters are required" - echo "Usage: launch_eloqdoc <bucket_name> <bucket_prefix>" + echo "Usage: launch_eloqdoc_fast <bucket_name> <bucket_prefix>" exit 1 fisrc/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (1)
654-656: Apply the same null guards as createTable (lines 519-524).The
updateTablefunction lacks the defensive null checks present increateTable. This inconsistency creates a null-dereference risk under race conditions or early initialization.Apply the same guard pattern used in
createTable:- auto* storeHandler = DataSubstrate::GetGlobal()->GetStoreHandler(); - std::string new_kv_info = - storeHandler->CreateNewKVCatalogInfo(tableName, currentTableSchema, alterTableInfo); + auto* ds = DataSubstrate::GetGlobal(); + invariant(ds != nullptr); + auto* storeHandler = ds->GetStoreHandler(); + invariant(storeHandler != nullptr); + std::string new_kv_info = + storeHandler->CreateNewKVCatalogInfo(tableName, currentTableSchema, alterTableInfo); + invariant(!new_kv_info.empty()); // or explicit error handling if empty is validsrc/mongo/db/modules/eloq/src/eloq_global_options.cpp (1)
36-40: Add missing gflags definitions to avoid linker errors.The
DECLARE_*statements create extern declarations without backing storage. OnlyDEFINE_string(data_substrate_config, ...)exists ineloq_kv_engine.cpp(line 135). The other three flags lack definitions, causing unresolved symbols at link time.Add these definitions in
src/mongo/db/modules/eloq/src/eloq_kv_engine.cppafter line 135:DEFINE_string(data_substrate_config, "", "Data Substrate Configuration"); DEFINE_int32(core_number, 0, "Number of cores reserved for Eloq workers (0=auto-detect)"); DEFINE_bool(bootstrap, false, "Bootstrap Eloq data substrate on startup"); DEFINE_bool(enable_data_store, true, "Enable Eloq data store services");src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (2)
127-135: Add the three missing gflags definitions here.Only
data_substrate_configis defined. The three flags declared ineloq_global_options.cpp(lines 37-39) lack backing storage, causing linker failures.Add after line 135:
DEFINE_string(data_substrate_config, "", "Data Substrate Configuration"); +DEFINE_int32(core_number, 0, "Number of cores reserved for Eloq workers (0=auto-detect)"); +DEFINE_bool(bootstrap, false, "Bootstrap Eloq data substrate on startup"); +DEFINE_bool(enable_data_store, true, "Enable Eloq data store services");
164-177: Based on my verification, I can now provide the rewritten review comment. The evidence strongly supports the original concern:
- In the same file (
eloq_kv_engine.cpplines 584-587 and 596-601), the shutdown code explicitly checksif (data_substrate != nullptr)after callingGetGlobal()- In other files (
eloq_recovery_unit.cpp,eloq_table_schema.cpp), the code addsinvariant(ds != nullptr)afterGetGlobal()- The constructor (lines 169-170) makes direct calls without any null checks, contradicting this pattern
This inconsistency indicates that
GetGlobal()can indeed return nullptr, making the constructor code unsafe.
Add null checks after DataSubstrate initialization to match defensive patterns used elsewhere in the codebase.
The constructor assumes
DataSubstrate::GetGlobal()returns a valid pointer without validation. However, the shutdown code in the same file (lines 584-587, 596-601) explicitly checksif (data_substrate != nullptr), and other code sites addinvariant()checks. This inconsistency indicates the pointer can be null.Add defensive checks:
log() << "Standalone mode: Initializing data substrate..."; DataSubstrate::InitializeGlobal(FLAGS_data_substrate_config); - -_logServer = DataSubstrate::GetGlobal()->GetLogServer(); -_txService = DataSubstrate::GetGlobal()->GetTxService(); +auto* ds = DataSubstrate::GetGlobal(); +invariant(ds != nullptr); +_logServer = ds->GetLogServer(); +invariant(_logServer != nullptr); +_txService = ds->GetTxService(); +invariant(_txService != nullptr);
🧹 Nitpick comments (4)
concourse/artifact/ELOQDSS_ROCKSDB/eloqdoc.conf (1)
21-24: Disabling session cache refresh and TTL monitor has operational impact
disableLogicalSessionCacheRefresh: trueandttlMonitorEnabled: falsewill prevent normal session cleanup and TTL index expiry. That’s often desirable for deterministic CI artifacts, but would be risky in long‑lived environments (potential unbounded growth of session/TTL collections). Consider adding a brief comment here or in the artifact docs clarifying that this config is CI‑only and ensuring production manifests leave these enabled.src/mongo/transport/service_entry_point_impl.cpp (1)
75-76: Debug log addition looks good; consider aligning log level with executor usage messageThe new
MONGO_LOG(1)in the constructor is useful for diagnosing why the coroutine executor is (or isn’t) created. Given thatstartSessionlogs"use coroutine service executor"at level 0, you might optionally want this config log at level 0 as well so operators at default verbosity can correlate the configuration with the behavior:- MONGO_LOG(1) << "enableCoroutine: " << serverGlobalParams.enableCoroutine - << ", reservedThreadNum: " << serverGlobalParams.reservedThreadNum; + MONGO_LOG(0) << "enableCoroutine: " << serverGlobalParams.enableCoroutine + << ", reservedThreadNum: " << serverGlobalParams.reservedThreadNum;If you intentionally keep it at level 1 to reduce noise, the current change is still fine as-is.
src/mongo/db/modules/eloq/SConscript (1)
307-313: Use env.get() to prevent KeyError on missing keys.Direct dictionary access like
env['MONGO_ALLOCATOR']on line 313 will raiseKeyErrorif the key is not set. Useenv.get()with a default value for safer debugging output.Apply this diff:
print("CPPDEFINES: ", env["CPPDEFINES"]) print("CPPPATH: ", env["CPPPATH"]) print("LIBPATH: ", env["LIBPATH"]) print("LIBS: ", env["LIBS"]) print("LINKFLAGS: ", env["LINKFLAGS"]) print("eloq_dependencies: ", eloq_dependencies) -print("MALLOC:", env['MONGO_ALLOCATOR']) +print("MALLOC:", env.get('MONGO_ALLOCATOR', '<unset>'))src/mongo/util/options_parser/options_parser.cpp (1)
512-515: Comment on composable options doc: mention StringMap as well as StringVectorThe comment says this “only works for options that are registered as vectors of strings”, but the implementation below also supports
StringMapoptions. Consider updating the wording to reflect bothStringVectorandStringMapto avoid confusion for future readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (66)
.gitignore(1 hunks).gitmodules(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB/data_substrate.cnf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate.cnf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate_cluster_a.cnf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate_cluster_b.cnf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate_cluster_c.cnf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_a.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_b.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_c.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdss.conf(1 hunks)concourse/pipeline/main.ent.yml(1 hunks)concourse/pipeline/main.yml(1 hunks)concourse/pipeline/pr.ent.yml(1 hunks)concourse/scripts/common.sh(7 hunks)concourse/scripts/data_substrate.cnf(1 hunks)concourse/scripts/main.bash(1 hunks)concourse/scripts/main.ent.bash(3 hunks)concourse/scripts/pr.ent.bash(3 hunks)concourse/scripts/store_rocksdb_cloud.yaml(1 hunks)concourse/tasks/main.ent.yml(0 hunks)concourse/tasks/main.yml(0 hunks)concourse/tasks/pr.ent.yml(0 hunks)src/mongo/SConscript(1 hunks)src/mongo/base/object_pool.h(1 hunks)src/mongo/db/dbmain.cpp(2 hunks)src/mongo/db/modules/eloq/CMakeLists.txt(2 hunks)src/mongo/db/modules/eloq/SConscript(6 hunks)src/mongo/db/modules/eloq/cmake/build_data_store.cmake(0 hunks)src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake(0 hunks)src/mongo/db/modules/eloq/cmake/build_eloq_metrics.cmake(0 hunks)src/mongo/db/modules/eloq/cmake/build_log_service.cmake(0 hunks)src/mongo/db/modules/eloq/cmake/build_tx_service.cmake(0 hunks)src/mongo/db/modules/eloq/cmake/compile_protos.cmake(0 hunks)src/mongo/db/modules/eloq/cmake/find_dependencies.cmake(0 hunks)src/mongo/db/modules/eloq/data_substrate(1 hunks)src/mongo/db/modules/eloq/eloq_metrics(0 hunks)src/mongo/db/modules/eloq/log_service(0 hunks)src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_key.h(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_table_schema.cpp(3 hunks)src/mongo/db/modules/eloq/src/base/eloq_table_schema.h(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_util.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_cursor.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_cursor.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_global_options.cpp(2 hunks)src/mongo/db/modules/eloq/src/eloq_global_options.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_index.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_index.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(8 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.h(3 hunks)src/mongo/db/modules/eloq/src/eloq_options_init.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_record_store.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_record_store.h(1 hunks)src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp(3 hunks)src/mongo/db/modules/eloq/src/eloq_recovery_unit.h(1 hunks)src/mongo/db/modules/eloq/store_handler(0 hunks)src/mongo/db/modules/eloq/tx_service(0 hunks)src/mongo/db/server_options.h(1 hunks)src/mongo/db/server_options_server_helpers.cpp(2 hunks)src/mongo/transport/service_entry_point_impl.cpp(1 hunks)src/mongo/transport/service_executor_coroutine.h(1 hunks)src/mongo/util/options_parser/options_parser.cpp(3 hunks)
💤 Files with no reviewable changes (14)
- src/mongo/db/modules/eloq/eloq_metrics
- src/mongo/db/modules/eloq/log_service
- concourse/tasks/pr.ent.yml
- src/mongo/db/modules/eloq/cmake/compile_protos.cmake
- src/mongo/db/modules/eloq/tx_service
- src/mongo/db/modules/eloq/cmake/build_eloq_metrics.cmake
- src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake
- src/mongo/db/modules/eloq/cmake/build_tx_service.cmake
- src/mongo/db/modules/eloq/cmake/build_data_store.cmake
- concourse/tasks/main.ent.yml
- src/mongo/db/modules/eloq/cmake/find_dependencies.cmake
- src/mongo/db/modules/eloq/store_handler
- concourse/tasks/main.yml
- src/mongo/db/modules/eloq/cmake/build_log_service.cmake
🚧 Files skipped from review as they are similar to previous changes (21)
- src/mongo/db/modules/eloq/src/eloq_record_store.h
- src/mongo/db/modules/eloq/data_substrate
- concourse/pipeline/main.ent.yml
- src/mongo/db/dbmain.cpp
- src/mongo/transport/service_executor_coroutine.h
- concourse/scripts/data_substrate.cnf
- src/mongo/db/modules/eloq/src/eloq_cursor.h
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdss.conf
- src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate_cluster_c.cnf
- src/mongo/db/modules/eloq/src/base/eloq_util.h
- src/mongo/db/server_options.h
- src/mongo/SConscript
- src/mongo/db/modules/eloq/src/eloq_options_init.cpp
- concourse/scripts/main.bash
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate_cluster_b.cnf
- concourse/pipeline/pr.ent.yml
- src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h
- src/mongo/base/object_pool.h
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf
- concourse/artifact/ELOQDSS_ROCKSDB/data_substrate.cnf
🧰 Additional context used
🧠 Learnings (5)
📚 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:
src/mongo/db/modules/eloq/src/eloq_global_options.hsrc/mongo/db/modules/eloq/src/eloq_global_options.cppsrc/mongo/util/options_parser/options_parser.cppsrc/mongo/db/modules/eloq/src/eloq_kv_engine.cppsrc/mongo/db/server_options_server_helpers.cpp
📚 Learning: 2025-11-24T03:11:24.955Z
Learnt from: lzxddz
Repo: eloqdata/eloqdoc PR: 325
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:1362-1384
Timestamp: 2025-11-24T03:11:24.955Z
Learning: The EloqDoc codebase uses an older version of glog that supports the two-parameter signature `google::InitGoogleLogging(const char* argv0, PrefixCallback)` for custom log prefix formatting, as seen in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp line 1383.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_global_options.hsrc/mongo/db/modules/eloq/src/eloq_recovery_unit.hsrc/mongo/db/modules/eloq/SConscriptsrc/mongo/db/modules/eloq/src/eloq_record_store.cppsrc/mongo/db/modules/eloq/src/eloq_global_options.cppsrc/mongo/db/modules/eloq/src/eloq_kv_engine.cppsrc/mongo/db/modules/eloq/src/eloq_kv_engine.hsrc/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp
📚 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:
src/mongo/db/modules/eloq/CMakeLists.txtsrc/mongo/db/modules/eloq/SConscriptsrc/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
📚 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:
src/mongo/db/modules/eloq/CMakeLists.txtsrc/mongo/db/modules/eloq/SConscriptconcourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_a.confsrc/mongo/db/modules/eloq/src/eloq_kv_engine.h
📚 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:
src/mongo/db/modules/eloq/SConscriptsrc/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
🧬 Code graph analysis (2)
src/mongo/db/modules/eloq/src/eloq_global_options.cpp (1)
src/mongo/db/server_options.h (1)
bootstrap(87-270)
src/mongo/db/modules/eloq/src/eloq_kv_engine.h (4)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (2)
EloqKVEngine(164-177)EloqKVEngine(179-184)src/mongo/base/string_data.h (2)
operator(182-184)operator(360-362)src/mongo/db/modules/eloq/src/base/eloq_table_schema.h (1)
- `` (305-310)
src/mongo/db/modules/eloq/src/base/eloq_util.h (1)
txservice(43-43)
🪛 Gitleaks (8.29.1)
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/data_substrate.cnf
[high] 29-29: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| #include "mongo/db/modules/eloq/data_substrate/tx_service/include/tx_execution.h" | ||
| #include "mongo/db/modules/eloq/data_substrate/tx_service/include/tx_record.h" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify headers exist at new location and check for path consistency
echo "=== 1. Checking if headers exist at new location ==="
fd -t f "tx_execution.h" mongo/db/modules/eloq/data_substrate/tx_service/include/ 2>/dev/null || echo "tx_execution.h not found"
fd -t f "tx_record.h" mongo/db/modules/eloq/data_substrate/tx_service/include/ 2>/dev/null || echo "tx_record.h not found"
echo -e "\n=== 2. Checking for any remaining references to old paths ==="
rg -n "mongo/db/modules/eloq/tx_service/include/(tx_execution|tx_record)" --type cpp --type h 2>/dev/null || echo "No old path references found"
echo -e "\n=== 3. Checking for new path references ==="
rg -n "mongo/db/modules/eloq/data_substrate/tx_service/include/(tx_execution|tx_record)" --type cpp --type h 2>/dev/null || echo "No new path references found"
echo -e "\n=== 4. Verifying eloq_cursor.cpp current include lines ==="
sed -n '25,26p' src/mongo/db/modules/eloq/src/eloq_cursor.cppRepository: eloqdata/eloqdoc
Length of output: 1309
🏁 Script executed:
#!/bin/bash
# Find where the actual header files are located
echo "=== Searching for tx_execution.h in entire codebase ==="
fd -t f "tx_execution.h" 2>/dev/null | head -20
echo -e "\n=== Searching for tx_record.h in entire codebase ==="
fd -t f "tx_record.h" 2>/dev/null | head -20
echo -e "\n=== Checking data_substrate directory structure ==="
fd -t d "data_substrate" 2>/dev/null | head -20
echo -e "\n=== Checking tx_service directory locations ==="
fd -t d "tx_service" 2>/dev/null | head -20Repository: eloqdata/eloqdoc
Length of output: 313
🏁 Script executed:
#!/bin/bash
# Explore data_substrate directory structure
echo "=== Contents of data_substrate directory ==="
find src/mongo/db/modules/eloq/data_substrate/ -type f -o -type d | head -50
echo -e "\n=== Checking eloq module structure ==="
find src/mongo/db/modules/eloq/ -maxdepth 2 -type d | sort
echo -e "\n=== Looking for any .h or .hpp files in eloq module ==="
fd -t f "\.(h|hpp)$" src/mongo/db/modules/eloq/ | head -30Repository: eloqdata/eloqdoc
Length of output: 1029
Includes reference non-existent headers—migration appears incomplete.
The verification reveals a critical issue: the headers referenced in lines 25-26 (tx_execution.h and tx_record.h) do not exist anywhere in the codebase. The data_substrate directory is empty, and neither tx_service nor the referenced headers exist at the new paths. All six files in the codebase that reference the new path will fail to compile due to missing includes.
This indicates the path migration in eloq_cursor.cpp (and the five other files) is broken and will cause compilation failures unless:
- The headers are generated at build time (undocumented)
- The headers need to be moved/created as part of this PR
- The includes should reference a different location
Action required: Verify that the header files exist in the repository or will be provided by the build system before merging this change.
🤖 Prompt for AI Agents
In src/mongo/db/modules/eloq/src/eloq_cursor.cpp around lines 25-26, the
includes reference tx_execution.h and tx_record.h under
data_substrate/tx_service/include which do not exist; to fix, verify and correct
the include paths or provide the headers: either change these include lines to
the original/actual header locations in the repo (search for
tx_execution/tx_record or their equivalents and update the relative include
paths), or add/move the missing header files into
data_substrate/tx_service/include as part of this PR (or ensure the build
generates them and document that); also update the other five files that
reference the same broken path to use the same corrected include locations or
added headers so the project compiles.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.