Conversation
WalkthroughUpdates Eloq KV engine bootstrap and datastore initialization call signatures and flow, adds ServiceContext::isStartupComplete() with an early-return in ReloadCache, and updates eloq submodule pointers for store_handler and tx_service. Changes
Sequence Diagram(s)sequenceDiagram
participant Engine as EloqKVEngine
participant Config as ConfigSource
participant DSS as DataStoreService
participant MSC as MongoSystemHandler
alt bootstrap
Engine->>Config: fetch/validate cluster configs -> tmpNgConfigs
Config-->>Engine: tmpNgConfigs
Engine->>DSS: initDataStoreService(nodeId, tmpNgConfigs)
else non-bootstrap
Engine->>DSS: initDataStoreService(nodeId, ngConfigs)
end
Note over DSS: build ng_leaders (bootstrap/single-node -> map to self)
DSS->>DSS: TxConfigsToDssClusterConfig(node_id, ngConfigs, ng_leaders, ds_config)
DSS->>DSS: StartService(bootstrap_or_singleNode_flag)
Note over MSC: ReloadCache guard
MSC->>MSC: if serviceContext==null || !isStartupComplete() -> return
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(4 hunks)src/mongo/db/modules/eloq/store_handler(1 hunks)src/mongo/db/modules/eloq/tx_service(1 hunks)src/mongo/db/service_context.cpp(1 hunks)src/mongo/db/service_context.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/mongo/db/service_context.h (1)
src/mongo/db/service_context.cpp (2)
isStartupComplete(394-397)isStartupComplete(394-394)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1)
src/mongo/db/server_options.h (1)
bootstrap(86-269)
🔇 Additional comments (1)
src/mongo/db/modules/eloq/tx_service (1)
1-1: All integration points have been correctly updated for the submodule API changes.Verification confirms that the parent repository's integration points are using the correct new API signatures:
- Line 727 (eloq_kv_engine.cpp):
DataStoreServiceClientconstructor correctly called with new boolean parameter(opt_bootstrap || isSingleNode)- Line 645 (eloq_kv_engine.cpp):
TxConfigsToDssClusterConfigcorrectly called with new signature(dss_node_id, ngConfigs, ng_leaders, ds_config)No stale API calls were found. The submodule update is properly integrated.
9d9e12c to
00c874b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1)
409-419: Critical: Search the wrong config map for node lookup.Line 409 iterates over
ngConfigs(the single-entry bootstrap config) instead oftmpNgConfigs(the full multi-shard config fetched at line 392). This causes the node lookup to fail on any host that isn't the first in the cluster, preventing bootstrap from completing on non-primary nodes.Apply this diff to fix the lookup:
bool found = false; uint32_t dssNodeId = UINT32_MAX; // check whether this node is in cluster. - for (auto& pair : ngConfigs) { + for (auto& pair : tmpNgConfigs) { auto& ngNodes = pair.second; for (auto& ngNode : ngNodes) { if (ngNode.host_name_ == eloqGlobalOptions.localAddr.host() &&Note: A similar issue was flagged in previous review comments and marked as addressed. This appears to be a regression or incomplete fix.
🧹 Nitpick comments (1)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1)
406-424: Refactor: Extract duplicate node-finding logic.The node-finding logic in lines 406-424 duplicates the logic in lines 363-386. Consider extracting this into a helper function to improve maintainability.
For example:
// Helper function to find node in config static std::optional<std::pair<uint32_t, uint32_t>> FindNodeInConfig( const std::unordered_map<uint32_t, std::vector<txservice::NodeConfig>>& configs, const std::string& hostName, uint16_t port) { for (auto& pair : configs) { auto& ngNodes = pair.second; for (auto& ngNode : ngNodes) { if (ngNode.host_name_ == hostName && ngNode.port_ == port) { uint32_t nodeId = ngNode.node_id_; uint32_t ngId = ngNode.is_candidate_ ? pair.first : 0; return std::make_pair(nodeId, ngId); } } } return std::nullopt; }Then use this helper at both line 366 and line 409.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(6 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.h(0 hunks)src/mongo/db/modules/eloq/store_handler(1 hunks)src/mongo/db/modules/eloq/tx_service(1 hunks)src/mongo/db/service_context.cpp(1 hunks)src/mongo/db/service_context.h(1 hunks)
💤 Files with no reviewable changes (1)
- src/mongo/db/modules/eloq/src/eloq_kv_engine.h
🚧 Files skipped from review as they are similar to previous changes (4)
- src/mongo/db/service_context.h
- src/mongo/db/modules/eloq/tx_service
- src/mongo/db/service_context.cpp
- src/mongo/db/modules/eloq/store_handler
🧰 Additional context used
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1)
src/mongo/db/server_options.h (1)
bootstrap(86-269)
🔇 Additional comments (7)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (7)
343-343: LGTM!Adding
trueas theis_candidate_parameter for the bootstrap solo config is correct, as the single node in bootstrap mode should be a candidate leader.
640-640: LGTM!The
isSingleNodecomputation correctly identifies single-node deployments.
657-663: LGTM!The
ng_leadersmap construction is correct for bootstrap/single-node scenarios. Each node group is assigned the current node as its initial leader, which is appropriate for starting all DSS shards locally during bootstrap.
665-666: LGTM!The updated
TxConfigsToDssClusterConfigsignature with per-node-group leaders (ng_leadersmap) properly supports multi-shard DSS configurations.
731-739: LGTM!The
StartServicecalls correctly differentiate between non-shared storage (RocksDB, alwayscreate_if_missing=true) and shared storage (only create during bootstrap or single-node mode to prevent conflicts).
746-750: LGTM!The
DataStoreServiceClientconstruction correctly passes the bootstrap/single-node flag as the first parameter, maintaining consistency with the updated signature and broader multi-shard initialization flow.
1338-1341: LGTM!The early-return guard prevents
ReloadCachefrom executing before startup completes, avoiding potential race conditions or errors when authorization data structures aren't yet initialized.
52f2abc to
2b9de01
Compare
fix single node start update bootstrap for dss dss client add arg bind_data_shard_with_ng update submodule
308b504 to
5c63e6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1)
662-671: Remove unary+before string literal inuassertedmessage (compile error)This expression:
uasserted(ErrorCodes::InternalError, +"DataStoreService initialization failed: unable to fetch config from peer " + ds_peer_node);uses unary
+on a string literal, which is not allowed in C++ and will not compile.You can fix it by dropping the unary
+:- uasserted(ErrorCodes::InternalError, - +"DataStoreService initialization failed: unable to fetch config from peer " + - ds_peer_node); + uasserted(ErrorCodes::InternalError, + "DataStoreService initialization failed: unable to fetch config from peer " + + ds_peer_node);(
"literal" + std::stringis valid via the standardoperator+overload.)
♻️ Duplicate comments (1)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1)
403-444: Fix bootstrap DSS node_id lookup to usetmpNgConfigs, notngConfigsIn bootstrap mode,
ngConfigsis a single-node placeholder (node_id_ always 0), whiletmpNgConfigsholds the real multi-shard topology loaded from file/IP list. ThedssNodeIdsearch and membership check currently iteratengConfigs, so every node ends up usingdssNodeId == 0when calling:initDataStoreService(dssNodeId, tmpNgConfigs);On nodes whose
(host, port)map to a differentnode_id_intmpNgConfigs, DSS cannot correctly identify the local node and cluster registration breaks (same underlying problem that was flagged earlier).Change the search to walk
tmpNgConfigs:- // check whether this node is in cluster. - for (auto& pair : ngConfigs) { + // check whether this node is in cluster. + for (auto& pair : tmpNgConfigs) { auto& ngNodes = pair.second; for (auto& ngNode : ngNodes) { if (ngNode.host_name_ == eloqGlobalOptions.localAddr.host() && ngNode.port_ == eloqGlobalOptions.localAddr.port()) { dssNodeId = ngNode.node_id_; found = true; break; } } }This ensures
dssNodeIdand the membership check reflect the real multi-shard config used byinitDataStoreService.
🧹 Nitpick comments (2)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (2)
651-683: DSS bootstrap/single-node wiring viang_leaders,StartService, and client flag looks coherent
- Using
isSingleNode = (ngConfigs.size() == 1 && ngConfigs.begin()->second.size() == 1)and then driving bothng_leaderspopulation and theStartService/DataStoreServiceClientboolean with(opt_bootstrap || isSingleNode)makes the intent clear: bootstrap or truly single-node deployments start all shards locally; multi-node non-bootstrap deployments rely on existing topology.- The new
TxConfigsToDssClusterConfig(node_id, ngConfigs, ng_leaders, ds_config)call is consistent with that contract.If you want to tidy it up, you could factor the repeated
(opt_bootstrap || isSingleNode)into a single localconst bool bootstrapOrSingleNodeand reuse it in the three call sites, but that’s purely cosmetic.Also applies to: 752-772
1374-1382: Startup guard inReloadCacheworks, but the null-check ongetGlobalServiceContext()is dead code
getGlobalServiceContext()fasserts if there is no globalServiceContext, so:auto serviceContext = mongo::getGlobalServiceContext(); if (serviceContext == nullptr || !serviceContext->isStartupComplete()) {will never see
serviceContext == nullptr. If you truly need to handle “no global context yet”, you’d have to checkhasGlobalServiceContext()before callinggetGlobalServiceContext(). Otherwise, you can simplify this to just checking!serviceContext->isStartupComplete()and also reuseserviceContextformakeClientinstead of callinggetGlobalServiceContext()again.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(7 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.h(0 hunks)src/mongo/db/modules/eloq/store_handler(1 hunks)src/mongo/db/modules/eloq/tx_service(1 hunks)src/mongo/db/service_context.cpp(1 hunks)src/mongo/db/service_context.h(1 hunks)
💤 Files with no reviewable changes (1)
- src/mongo/db/modules/eloq/src/eloq_kv_engine.h
✅ Files skipped from review due to trivial changes (1)
- src/mongo/db/modules/eloq/store_handler
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mongo/db/modules/eloq/tx_service
🧰 Additional context used
🧠 Learnings (1)
📚 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
🧬 Code graph analysis (2)
src/mongo/db/service_context.h (1)
src/mongo/db/service_context.cpp (2)
isStartupComplete(394-397)isStartupComplete(394-394)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1)
src/mongo/db/server_options.h (1)
bootstrap(86-269)
🔇 Additional comments (3)
src/mongo/db/service_context.h (1)
445-446: isStartupComplete accessor pairs cleanly with existing startup APIsThe new declaration is positioned appropriately next to wait/notify and matches the implementation; no API or threading concerns here.
src/mongo/db/service_context.cpp (1)
394-397: Thread-safe read of startup state looks correctLocking
_mutexand reading_startupCompletematches the existing wait/notify pattern and safely exposes the flag.src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1)
355-360: Solo bootstrap NodeConfig now correctly marks local node as candidatePassing
truefor the solo-nodeNodeConfigkeepsnativeNgIddiscovery and candidate semantics consistent during bootstrap.
Summary by CodeRabbit
Refactor
Bug Fixes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.