Conversation
|
Caution Review failedThe pull request is closed. WalkthroughConsolidates DataSubstrate configuration and Google Logging initialization into Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Init as Process startup
participant Global as EloqGlobalOptions
participant DataSub as DataSubstrate::Instance()
participant GLog as Google Logging
participant SGP as serverGlobalParams
Init->>Global: call global options init
Global->>GLog: determine log_dir from SGP.logpath parent
Global->>GLog: InitGoogleLogging(argv_synthetic)
Global->>DataSub: Init(FLAGS_data_substrate_config)
DataSub-->>Global: return CoreConfig { bootstrap, core_num }
Global->>SGP: set bootstrap = CoreConfig.bootstrap
Global->>SGP: set reservedThreadNum = CoreConfig.core_num
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/mongo/db/modules/eloq/src/eloq_global_options.cpp:
- Around line 82-85: DataSubstrate::Instance().Init(...) is called without
checking its return value; modify the code around
DataSubstrate::Instance().Init(FLAGS_data_substrate_config) to capture its
Status/return value, check for non-OK/failure, and on error log an explanatory
message and return that non-OK Status (or otherwise propagate the error) instead
of proceeding to call GetCoreConfig(); ensure subsequent uses like
DataSubstrate::Instance().GetCoreConfig(), serverGlobalParams.bootstrap and
serverGlobalParams.reservedThreadNum only run when Init succeeded.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mongo/db/modules/eloq/src/eloq_global_options.cppsrc/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
💤 Files with no reviewable changes (1)
- src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Learnt from: 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.
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.
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: concourse/scripts/main.ent.bash:38-39
Timestamp: 2025-11-28T10:52:25.527Z
Learning: In the eloqdoc enterprise build (main.ent.bash), eloq_log_service is fetched separately as the resource `eloq_logservice_src` in the pipeline (main.ent.yml) and then symlinked into the data_substrate directory structure at build time using `ln -s $WORKSPACE/eloq_logservice_src eloq_log_service`. This is the intended architecture for placing eloq_log_service within data_substrate in the enterprise configuration.
📚 Learning: 2025-11-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.cpp
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_global_options.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.cpp
🔇 Additional comments (4)
src/mongo/db/modules/eloq/src/eloq_global_options.cpp (4)
24-24: LGTM!The new includes are appropriate for the added filesystem and logging functionality.
Also applies to: 28-28
37-37: LGTM!The gflags definition uses the appropriate
data_substrate_prefix as per repository conventions. Based on learnings, this avoids conflicts with MongoDB'soptions_parserwhich owns--config.
72-77: LGTM!The
staticqualifier onlogdirensures the path persists for the lifetime needed bySetCommandLineOption. Thehas_parent_path()check correctly handles edge cases wherelogpathmight be empty or a filename without a directory component.
78-80: No issue found.EloqGlobalOptions::store()is called exactly once during startup via theMONGO_STARTUP_OPTIONS_STOREinitialization macro. It is not invoked during config reloads or in any scenario that would causeInitGoogleLoggingto be called multiple times.
| DataSubstrate::Instance().Init(FLAGS_data_substrate_config); | ||
| serverGlobalParams.bootstrap = DataSubstrate::Instance().GetCoreConfig().bootstrap; | ||
| MONGO_LOG(1) << "serverGlobalParams.bootstrap: " << serverGlobalParams.bootstrap; | ||
|
|
||
| const char* field_core = "core_number"; | ||
| uint32_t core_num = FLAGS_core_number; | ||
| if (CheckCommandLineFlagIsDefault(field_core)) { | ||
| if (ds_config_reader.HasValue("local", field_core)) { | ||
| core_num = ds_config_reader.GetInteger("local", field_core, 0); | ||
| assert(core_num); | ||
| } else { | ||
| if (!NUM_VCPU) { | ||
| MONGO_LOG(1) << "config is missing: " << field_core; | ||
| return Status{ErrorCodes::InvalidOptions, | ||
| str::stream() << "config is missing: " << field_core}; | ||
| } | ||
| const uint min = 1; | ||
| if (enable_data_store) { | ||
| core_num = std::max(min, (NUM_VCPU * 3) / 5); | ||
| MONGO_LOG(1) << "give cpus to checkpointer " << core_num; | ||
| } else { | ||
| core_num = std::max(min, (NUM_VCPU * 7) / 10); | ||
| } | ||
| MONGO_LOG(1) << "config is automatically set: " << field_core << "=" << core_num | ||
| << ", vcpu=" << NUM_VCPU; | ||
| } | ||
| } | ||
| serverGlobalParams.reservedThreadNum = core_num; | ||
| serverGlobalParams.reservedThreadNum = DataSubstrate::Instance().GetCoreConfig().core_num; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file and the specific lines mentioned
cat -n src/mongo/db/modules/eloq/src/eloq_global_options.cpp | head -100 | tail -30Repository: eloqdata/eloqdoc
Length of output: 1607
🏁 Script executed:
# Search for DataSubstrate class definition and Init method
rg -n "class DataSubstrate" --type cppRepository: eloqdata/eloqdoc
Length of output: 42
🏁 Script executed:
# Find where DataSubstrate::Init is defined
rg -n "Init\(" --type cpp -A 5 | grep -A 5 "DataSubstrate"Repository: eloqdata/eloqdoc
Length of output: 1066
🏁 Script executed:
# Look for DataSubstrate::Instance().Init() usage patterns in the codebase
rg -n "DataSubstrate::Instance\(\)\.Init" --type cpp -B 2 -A 3Repository: eloqdata/eloqdoc
Length of output: 771
🏁 Script executed:
# Check the complete function that contains lines 82-85
sed -n '70,95p' src/mongo/db/modules/eloq/src/eloq_global_options.cppRepository: eloqdata/eloqdoc
Length of output: 1059
🏁 Script executed:
# Find DataSubstrate class definition
rg -n "class DataSubstrate" --type cpp -A 20Repository: eloqdata/eloqdoc
Length of output: 42
🏁 Script executed:
# Search for Init method definition in DataSubstrate
rg -n "DataSubstrate.*Init\(" --type cpp -B 3 -A 10Repository: eloqdata/eloqdoc
Length of output: 1446
🏁 Script executed:
# Check for DataSubstrate.h header files
find . -name "*data_substrate*" -type fRepository: eloqdata/eloqdoc
Length of output: 444
🏁 Script executed:
# Look for Init method with signature
ast-grep --pattern 'Init($$$)'Repository: eloqdata/eloqdoc
Length of output: 1461
🏁 Script executed:
# Search for files containing DataSubstrate
fd ".*" --type f | xargs rg -l "class DataSubstrate" 2>/dev/nullRepository: eloqdata/eloqdoc
Length of output: 44
🏁 Script executed:
# Search in the eloq directory specifically
fd ".*" src/mongo/db/modules/eloq --type f | xargs rg -l "DataSubstrate" 2>/dev/null | head -20Repository: eloqdata/eloqdoc
Length of output: 319
🏁 Script executed:
# Look for DataSubstrate header file
find src -name "*.h" -o -name "*.hpp" | xargs rg "class DataSubstrate" 2>/dev/nullRepository: eloqdata/eloqdoc
Length of output: 44
🏁 Script executed:
# Check what files are in the eloq directory
find src/mongo/db/modules/eloq -name "*.h" -o -name "*.cpp" | head -30Repository: eloqdata/eloqdoc
Length of output: 1433
🏁 Script executed:
# Search all cpp and h files for DataSubstrate class definition
rg "class DataSubstrate" --type cpp --type h -A 30Repository: eloqdata/eloqdoc
Length of output: 42
🏁 Script executed:
# Search for DataSubstrate::Instance in includes or other patterns
rg "DataSubstrate" --type cpp --type h | head -40Repository: eloqdata/eloqdoc
Length of output: 1489
🏁 Script executed:
# Check the includes in eloq_global_options.cpp
head -50 src/mongo/db/modules/eloq/src/eloq_global_options.cppRepository: eloqdata/eloqdoc
Length of output: 1970
🏁 Script executed:
# Find the data_substrate.h header file
find . -name "data_substrate.h" -type fRepository: eloqdata/eloqdoc
Length of output: 42
🏁 Script executed:
# Check if it's in a third-party or external directory
find . -path "*data_substrate*" -name "*.h" -type fRepository: eloqdata/eloqdoc
Length of output: 42
🏁 Script executed:
# Look for the Init method signature in the header
grep -r "Init(" src/mongo/db/modules/eloq/data_substrate --include="*.h" -A 5 2>/dev/null | head -40Repository: eloqdata/eloqdoc
Length of output: 42
🏁 Script executed:
# Check what's in the data_substrate directory
ls -la src/mongo/db/modules/eloq/data_substrate/ 2>/dev/null || echo "Directory doesn't exist or is empty"Repository: eloqdata/eloqdoc
Length of output: 168
🏁 Script executed:
# Check if data_substrate is a git submodule or external dependency
cat .gitmodules 2>/dev/null | grep data_substrate || echo "Not found in gitmodules"Repository: eloqdata/eloqdoc
Length of output: 163
🏁 Script executed:
# Check all includes in eloq_global_options.cpp
grep -n "#include" src/mongo/db/modules/eloq/src/eloq_global_options.cppRepository: eloqdata/eloqdoc
Length of output: 663
🏁 Script executed:
# Search for where Init return type might be documented
rg "void.*Init\|Status.*Init\|bool.*Init" src/mongo/db/modules/eloq --type cpp --type h -A 3Repository: eloqdata/eloqdoc
Length of output: 42
🏁 Script executed:
# Check if there's a return value check pattern elsewhere in similar initialization code
rg "Instance\(\)\.Init" src/mongo/db/modules/eloq --type cpp -B 2 -A 5Repository: eloqdata/eloqdoc
Length of output: 980
🏁 Script executed:
# Check if there are any comments or documentation about Init() in the eloq files
rg -n "Init" src/mongo/db/modules/eloq/src/eloq_global_options.cpp -B 3 -A 3Repository: eloqdata/eloqdoc
Length of output: 556
🏁 Script executed:
# Check if the include path gives us any clues
grep -n "data_substrate.h" src/mongo/db/modules/eloq/src/eloq_global_options.cppRepository: eloqdata/eloqdoc
Length of output: 140
🏁 Script executed:
# Look at eloq_kv_engine.cpp to see if there's any Init() error handling elsewhere
grep -n "Init\|Shutdown\|EnableEngine" src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp -B 2 -A 3Repository: eloqdata/eloqdoc
Length of output: 1299
🏁 Script executed:
# Check for Status return patterns in eloq_global_options.cpp
grep -n "Status" src/mongo/db/modules/eloq/src/eloq_global_options.cpp | head -20Repository: eloqdata/eloqdoc
Length of output: 275
🏁 Script executed:
# Check if there's any documentation or comments about error handling expectations
grep -rn "error\|Error\|fail\|Fail" src/mongo/db/modules/eloq/src/eloq_global_options.cppRepository: eloqdata/eloqdoc
Length of output: 201
🏁 Script executed:
# Look at the function signature to understand context
sed -n '55,90p' src/mongo/db/modules/eloq/src/eloq_global_options.cppRepository: eloqdata/eloqdoc
Length of output: 1793
🏁 Script executed:
# Check if there are try-catch blocks anywhere in the function
grep -n "try\|catch" src/mongo/db/modules/eloq/src/eloq_global_options.cppRepository: eloqdata/eloqdoc
Length of output: 42
Add error handling for DataSubstrate initialization.
The DataSubstrate::Instance().Init() call on line 82 does not check for failure, yet the function returns Status and demonstrates error handling elsewhere (line 68). If initialization fails, the function still returns Status::OK() and subsequent GetCoreConfig() calls may operate on uninitialized state, allowing the server to start with invalid configuration.
Suggested fix: Add error handling
- DataSubstrate::Instance().Init(FLAGS_data_substrate_config);
+ auto init_status = DataSubstrate::Instance().Init(FLAGS_data_substrate_config);
+ if (!init_status) { // Adjust based on actual return type
+ return Status{ErrorCodes::InternalError,
+ str::stream() << "Failed to initialize DataSubstrate"};
+ }Note: The exact error handling pattern depends on how Init() signals failure. Refer to the DataSubstrate header for the correct return type and error condition check.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DataSubstrate::Instance().Init(FLAGS_data_substrate_config); | |
| serverGlobalParams.bootstrap = DataSubstrate::Instance().GetCoreConfig().bootstrap; | |
| MONGO_LOG(1) << "serverGlobalParams.bootstrap: " << serverGlobalParams.bootstrap; | |
| const char* field_core = "core_number"; | |
| uint32_t core_num = FLAGS_core_number; | |
| if (CheckCommandLineFlagIsDefault(field_core)) { | |
| if (ds_config_reader.HasValue("local", field_core)) { | |
| core_num = ds_config_reader.GetInteger("local", field_core, 0); | |
| assert(core_num); | |
| } else { | |
| if (!NUM_VCPU) { | |
| MONGO_LOG(1) << "config is missing: " << field_core; | |
| return Status{ErrorCodes::InvalidOptions, | |
| str::stream() << "config is missing: " << field_core}; | |
| } | |
| const uint min = 1; | |
| if (enable_data_store) { | |
| core_num = std::max(min, (NUM_VCPU * 3) / 5); | |
| MONGO_LOG(1) << "give cpus to checkpointer " << core_num; | |
| } else { | |
| core_num = std::max(min, (NUM_VCPU * 7) / 10); | |
| } | |
| MONGO_LOG(1) << "config is automatically set: " << field_core << "=" << core_num | |
| << ", vcpu=" << NUM_VCPU; | |
| } | |
| } | |
| serverGlobalParams.reservedThreadNum = core_num; | |
| serverGlobalParams.reservedThreadNum = DataSubstrate::Instance().GetCoreConfig().core_num; | |
| auto init_status = DataSubstrate::Instance().Init(FLAGS_data_substrate_config); | |
| if (!init_status) { // Adjust based on actual return type | |
| return Status{ErrorCodes::InternalError, | |
| str::stream() << "Failed to initialize DataSubstrate"}; | |
| } | |
| serverGlobalParams.bootstrap = DataSubstrate::Instance().GetCoreConfig().bootstrap; | |
| MONGO_LOG(1) << "serverGlobalParams.bootstrap: " << serverGlobalParams.bootstrap; | |
| serverGlobalParams.reservedThreadNum = DataSubstrate::Instance().GetCoreConfig().core_num; |
🤖 Prompt for AI Agents
In @src/mongo/db/modules/eloq/src/eloq_global_options.cpp around lines 82 - 85,
DataSubstrate::Instance().Init(...) is called without checking its return value;
modify the code around
DataSubstrate::Instance().Init(FLAGS_data_substrate_config) to capture its
Status/return value, check for non-OK/failure, and on error log an explanatory
message and return that non-OK Status (or otherwise propagate the error) instead
of proceeding to call GetCoreConfig(); ensure subsequent uses like
DataSubstrate::Instance().GetCoreConfig(), serverGlobalParams.bootstrap and
serverGlobalParams.reservedThreadNum only run when Init succeeded.
Eloqdoc should not read directly from data substrate config file. It should read from data substrate.
Eloqdoc should not read directly from data substrate config file. It should read from data substrate.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.