Skip to content

Remove mutex from DataStoreService#225

Merged
lzxddz merged 2 commits intoeloqdata:mainfrom
lzxddz:remove-mutex
Oct 14, 2025
Merged

Remove mutex from DataStoreService#225
lzxddz merged 2 commits intoeloqdata:mainfrom
lzxddz:remove-mutex

Conversation

@lzxddz
Copy link
Collaborator

@lzxddz lzxddz commented Sep 30, 2025

Summary by CodeRabbit

  • New Features
    • Introduced single-service startup for the data store with support for bootstrap/single-node mode.
  • Refactor
    • Consolidated startup flow into a unified service, removing per-shard initialization and connections.
    • Simplified success/failure handling to rely on a single startup result.
  • Chores
    • Updated internal module reference for the store handler submodule.

@lzxddz lzxddz self-assigned this Sep 30, 2025
@coderabbitai
Copy link

coderabbitai bot commented Sep 30, 2025

Caution

Review failed

The pull request is closed.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The title claims removal of a mutex from DataStoreService, but the diff instead restructures the datastore startup flow into a unified StartService call with a bootstrap flag and removes per-shard initialization, with no mutex removal present. It therefore misrepresents the main change. Please update the title to summarize the actual change, for example “Consolidate DataStoreService startup with bootstrap flag” or “Unify per-shard datastore initialization into DataStoreService::StartService(bool bootstrapOrSingleNode)”.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4d4d5e5 and 78add2e.

📒 Files selected for processing (2)
  • src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1 hunks)
  • src/mongo/db/modules/eloq/store_handler (1 hunks)

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

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

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)

554-658: Hardcoded is_single_node forces StartService into bootstrap/single-node mode
is_single_node is set to true (line 556), so (opt_bootstrap || is_single_node) always yields true and skips proper multi-node initialization. Implement real single-node detection (e.g. ngConfigs.size()==1 && ngConfigs[0].size()==1), remove the TODO, and verify multi-node startup.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 077a8d2 and 886a96f.

📒 Files selected for processing (3)
  • src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1 hunks)
  • src/mongo/db/modules/eloq/store_handler (1 hunks)
  • src/mongo/db/modules/eloq/tx_service (1 hunks)
🔇 Additional comments (1)
src/mongo/db/modules/eloq/store_handler (1)

1-1: Validate submodule commit and changes
The submodule update from b3e8d0e6638553e8000fb512db73c1f3704e09e4 to 57d1ffcb10bac1517b68be745d6fe6da0b757f92 could not be fetched locally—confirm both SHAs exist on the remote and verify that the changes in store_handler remove the mutex in DataStoreService and integrate with the new centralized StartService API.

Copy link
Collaborator

@githubzilla githubzilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

579-583: Implement dynamic single-node detection instead of hardcoding
The stub bool is_single_node = true; (src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:581) causes StartService(opt_bootstrap || is_single_node) to always evaluate true, breaking multi-node startup. Replace with a real cluster-topology check before merging.

🧹 Nitpick comments (1)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1)

682-683: Refine the comment for clarity.

The comment "the data store will start data store if needed" contains redundant phrasing.

Apply this diff to improve clarity:

-    // setup local data store service, the data store will start data store if needed.
+    // Setup local data store service. The service will start the data store if needed.
     bool ret = Eloq::dataStoreService->StartService((opt_bootstrap || is_single_node));
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 886a96f and 4d4d5e5.

📒 Files selected for processing (3)
  • src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1 hunks)
  • src/mongo/db/modules/eloq/store_handler (1 hunks)
  • src/mongo/db/modules/eloq/tx_service (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • src/mongo/db/modules/eloq/tx_service
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/mongo/db/modules/eloq/store_handler

@lzxddz lzxddz merged commit 65f4912 into eloqdata:main Oct 14, 2025
1 check passed
This was referenced Nov 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants