refactor init process

[liunyl]

Summary by CodeRabbit

• Refactor

  • Standardized internal initialization and handler access across core modules for improved consistency and maintainability.
  • Simplified shutdown and lifecycle handling to reduce complexity.

• Chores

  • Updated submodule reference to a newer commit.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Warning

Rate limit exceeded

@liunyl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 8 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 2067b75 and 5fb4c4c.

📒 Files selected for processing (5)

• src/mongo/db/modules/eloq/data_substrate (1 hunks)
• src/mongo/db/modules/eloq/src/base/eloq_table_schema.cpp (2 hunks)
• src/mongo/db/modules/eloq/src/base/eloq_util.h (1 hunks)
• src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (16 hunks)
• src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (2 hunks)

Walkthrough

Refactors Eloq to replace global DataSubstrate accessors with a singleton-based API, updates initialization/shutdown to use the singleton, removes two global pointer declarations, and updates the data_substrate submodule pointer.

Changes

Cohort / File(s)	Change Summary
Submodule Update src/mongo/db/modules/eloq/data_substrate	Submodule pointer updated to commit 347e42ee31eddf6d5ae3b24af84a60524643ac17.
Store Handler Accessor Migration src/mongo/db/modules/eloq/src/base/eloq_table_schema.cpp, src/mongo/db/modules/eloq/src/base/eloq_util.h, src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp	Replaced DataSubstrate::GetGlobal()->GetStoreHandler() with DataSubstrate::Instance().GetStoreHandler() in constructors, utilities, and recovery-unit methods; no other logic changes.
Initialization, Shutdown & Global Pointer Removal src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp	Replaced DataSubstrate::InitializeGlobal(...) with DataSubstrate::Instance().Init(...); changed shutdown calls to DataSubstrate::Instance().Shutdown(); obtain log server and tx service from the singleton; removed global pointers eloqdoc_catalog_factory and eloqdoc_system_handler.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Pay attention to initialization and shutdown sequencing in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp.
• Confirm DataSubstrate::Instance() is thread-safe and fully initialized before use at all call sites.
• Search codebase for any remaining uses of the removed global pointers or GetGlobal() calls.

Possibly related PRs

• Converge support eloqdoc #279 — Related refactor migrating Eloq codepaths to use the DataSubstrate entrypoint and replacing GetGlobal() accessors with the singleton pattern.

Suggested reviewers

• xiexiaoy
• lzxddz

Poem

🐰 I hopped from globals into one cozy den,
Instance here, instance there—no scattered pins again.
Submodule bumped, pointers dropped, init and shutdown shine,
A tidy little burrow where the data hops in line. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'refactor init process' accurately describes the main change: migrating from global initialization to singleton-based initialization pattern across multiple files.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

src/mongo/db/modules/eloq/src/base/eloq_util.h (1)

49-50: Consider adding a null check for storeHandler for defensive consistency.

Other call sites in this PR (e.g., MongoTableSchema constructor at line 250 and createTable at line 520 of eloq_recovery_unit.cpp) add invariant(storeHandler != nullptr) after calling GetStoreHandler(). This call site directly uses the pointer without validation.

     auto* storeHandler = DataSubstrate::Instance().GetStoreHandler();
+    invariant(storeHandler != nullptr);
     bool success = storeHandler->DiscoverAllTableNames(tables, yieldFuncPtr, resumeFuncPtr);

src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (2)

165-176: Inconsistent use of ds reference.

Line 167 creates a local reference ds to the singleton, but line 169 calls DataSubstrate::Instance().EnableEngine(...) directly instead of using ds.EnableEngine(...). For consistency and to avoid redundant singleton lookups, use the ds reference throughout.

     DataSubstrate::Instance().Init(FLAGS_data_substrate_config);

     auto &ds = DataSubstrate::Instance();

-    DataSubstrate::Instance().EnableEngine(txservice::TableEngine::EloqDoc);
+    ds.EnableEngine(txservice::TableEngine::EloqDoc);

     ds.RegisterEngine(txservice::TableEngine::EloqDoc, &catalogFactory, &mongoSystemHandler, {}, {});
     ds.Start();

---

174-175: Consider validating returned service handles.

GetLogServer() and GetTxService() results are assigned to member variables without validation. If the substrate fails to initialize properly, these could be null. Later code (e.g., newRecoveryUnit at line 202) uses invariant(_txService != nullptr), but an early check here could provide clearer error diagnostics at startup.

     _logServer = ds.GetLogServer();
     _txService = ds.GetTxService();
+    invariant(_txService != nullptr, "TxService failed to initialize");

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 69a528f and 21133de.

📒 Files selected for processing (5)

• src/mongo/db/modules/eloq/data_substrate (1 hunks)
• src/mongo/db/modules/eloq/src/base/eloq_table_schema.cpp (2 hunks)
• src/mongo/db/modules/eloq/src/base/eloq_util.h (1 hunks)
• src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (3 hunks)
• src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (2 hunks)

🧰 Additional context used

🧠 Learnings (5)

📓 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.

📚 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/base/eloq_util.h
• src/mongo/db/modules/eloq/src/base/eloq_table_schema.cpp
• src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
• src/mongo/db/modules/eloq/data_substrate

📚 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_kv_engine.cpp

📚 Learning: 2025-11-28T10:52:25.527Z

Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: concourse/scripts/main.ent.bash:38-39
Timestamp: 2025-11-28T10:52:25.527Z
Learning: In the eloqdoc enterprise build (main.ent.bash), eloq_log_service is fetched separately as the resource `eloq_logservice_src` in the pipeline (main.ent.yml) and then symlinked into the data_substrate directory structure at build time using `ln -s $WORKSPACE/eloq_logservice_src eloq_log_service`. This is the intended architecture for placing eloq_log_service within data_substrate in the enterprise configuration.

Applied to files:

• src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp

📚 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

🔇 Additional comments (6)

src/mongo/db/modules/eloq/data_substrate (1)

1-1: Submodule pointer update is valid and aligns with PR objectives.

The commit hash 947024253261d63806c24ad36156bdd096ad16ca points to a valid commit in the tx_service submodule repository (https://github.com/eloqdata/tx_service.git). No CMakeLists.txt or build configuration changes were made in this submodule commit, confirming that CMake configuration options (EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, STATISTICS) are preserved as required.

src/mongo/db/modules/eloq/src/base/eloq_table_schema.cpp (2)

249-252: LGTM!

The singleton accessor change is correctly applied with proper null safety check via invariant(storeHandler != nullptr).

---

308-310: LGTM!

Consistent with the constructor change, the null check is properly in place.

src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (1)

519-522: LGTM!

The singleton accessor change is correctly applied with proper null safety check.

src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (2)

589-589: LGTM!

The singleton shutdown call is correctly applied for the non-merged mode path.

---

594-601: LGTM!

The threaded shutdown path correctly uses the singleton pattern. The mutex/condition variable synchronization ensures proper shutdown coordination when called from a brpc-worker thread.