Remove mutex from DataStoreService by lzxddz · Pull Request #107 · eloqdata/eloqsql

lzxddz · 2025-09-07T04:18:48Z

Summary by CodeRabbit

Refactor
- Consolidated data store startup into a single service-managed flow, reducing startup branches and simplifying initialization.
- Simplified shutdown/reset behavior for cleaner service teardown and improved startup stability.
Chores
- Updated an internal submodule reference with no functional impact.

coderabbitai · 2025-09-30T07:48:19Z

Caution

Review failed

The pull request is closed.

Walkthrough

Removed per-shard DataStore creation, Initialize/StartDB, and per-shard mappings from storage/eloq/ha_eloq.cc; startup now calls data_store_service_->StartService(opt_bootstrap || is_single_node). Shutdown no longer calls DisconnectDataStore, only resets data_store_service_. store_handler submodule commit updated only.

Changes

Cohort / File(s)	Summary
Eloq data store startup refactor `storage/eloq/ha_eloq.cc`	Removed shard-specific DataStore creation, `Initialize`/`StartDB`, `dss_shards` and `dss_shards_map` usage. Replaced with centralized `data_store_service_->StartService(opt_bootstrap
Submodule pointer update `storage/eloq/store_handler`	Submodule commit hash updated only; no functional, API, or control-flow changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Server
  participant ha_eloq
  participant DSS as data_store_service_

  rect rgba(230,250,255,0.6)
  note over ha_eloq,DSS: Startup
  Server->>ha_eloq: Initialize()
  alt bootstrap or single-node
    ha_eloq->>DSS: StartService(true)
  else normal
    ha_eloq->>DSS: StartService(false)
  end
  DSS-->>ha_eloq: Service ready
  ha_eloq-->>Server: Initialization complete
  end

  rect rgba(255,240,230,0.6)
  note over ha_eloq,DSS: Shutdown
  Server->>ha_eloq: eloq_done_func()
  ha_eloq->>ha_eloq: reset data_store_service_ (no DisconnectDataStore)
  end

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

liunyl

Poem

A rabbit taps keys as morning gleams,
Shards step back while one service dreams.
One tidy start, one gentle reset,
Quiet code hops — no loose threads yet. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Title Check	⚠️ Warning	The title “Remove mutex from DataStoreService” does not align with the primary changes in this pull request, which center on eliminating shard-specific data store setup and simplifying the DataStoreService startup flow rather than removing any mutex. This mismatch could mislead reviewers about the actual scope of the refactoring. Because the title fails to summarize the core change described in the diffs and summaries, it does not meet the criteria for a clear and accurate PR title.	Please update the title to clearly reflect the main change, such as “Simplify DataStoreService startup by removing per-shard initialization” or a similarly concise summary of removing shard-specific data store setup.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

📜 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 f9cf553 and 10e5912.

📒 Files selected for processing (2)

storage/eloq/ha_eloq.cc (1 hunks)
storage/eloq/store_handler (1 hunks)

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific 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

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

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 211f9e9 and 1aaa871.

📒 Files selected for processing (3)

storage/eloq/ha_eloq.cc (1 hunks)
storage/eloq/store_handler (1 hunks)
storage/eloq/tx_service (1 hunks)

🔇 Additional comments (2)

storage/eloq/tx_service (1)

1-1: Manual verification required for storage/eloq/tx_service submodule update

I couldn’t fetch the diff between commits 2bd068afeac7e7621c15bd7ac718f6e96cd5ce73 → 4c571f85afe887d79f40d259efaa3601083a9ab2 locally; please review the changes in storage/eloq/tx_service to ensure:

Removed mutex logic doesn’t introduce new concurrency issues

Submodule API remains compatible with the simplified DataStoreService::StartService call in ha_eloq.cc

Any breaking changes in tx_service are properly handled in the main codebase

storage/eloq/store_handler (1)

1-1: Verify submodule commit details manually.
Local inspection of commit 57d1ffcb10bac1517b68be745d6fe6da0b757f92 failed due to missing history; ensure this submodule update implements the mutex removal in DataStoreService and supports the new data_store_service_->StartService() pattern.

coderabbitai · 2025-09-30T07:51:53Z

storage/eloq/ha_eloq.cc

+    // setup local data store service, the data store service will start
+    // data store if needed.
+    bool ret=
+        data_store_service_->StartService((opt_bootstrap || is_single_node));
    if (!ret)


⚠️ Potential issue | 🔴 Critical

is_single_node is always true, so clustered nodes now take the single-node start path.

is_single_node is initialized to true above and never flipped to false when we fetch topology from a peer. After this change the expression (opt_bootstrap || is_single_node) therefore always evaluates to true, so every node (including multi-node deployments) now forces the single-node StartService path. That regresses clustered setups that previously relied on the service staying in follower mode. Please compute the actual single-node state (e.g., from the loaded ds_config) before invoking StartService, or pass the existing flag that already encodes it, so clustered nodes don’t wrongly bootstrap themselves.

🤖 Prompt for AI Agents

In storage/eloq/ha_eloq.cc around lines 2705-2709, is_single_node is initialized to true earlier and never updated, so the expression (opt_bootstrap || is_single_node) always evaluates to true and forces single-node StartService; update the code to compute the real single-node state before calling StartService by deriving it from the loaded ds_config (or use the existing flag that already encodes cluster membership) — e.g., set is_single_node = (ds_config indicates no peers / cluster size == 1) or use the provided cluster flag, then call data_store_service_->StartService((opt_bootstrap || is_single_node)) so only genuine single-node deployments take the single-node path.

githubzilla

+1

lzxddz self-assigned this Sep 7, 2025

lzxddz added the trigger-ci label Sep 7, 2025

lzxddz mentioned this pull request Sep 7, 2025

Remove lock serv_mux_ from DataStoreService eloqdata/store_handler#68

Merged

lzxddz force-pushed the remove-mutex branch from 363e8bd to 7857742 Compare September 11, 2025 08:31

lzxddz force-pushed the remove-mutex branch from 7857742 to 1aaa871 Compare September 30, 2025 07:48

coderabbitai bot reviewed Sep 30, 2025

View reviewed changes

githubzilla approved these changes Oct 13, 2025

View reviewed changes

lzxddz force-pushed the remove-mutex branch from 1aaa871 to 480544d Compare October 13, 2025 09:03

lzxddz added 2 commits October 14, 2025 15:22

Start datastore in DataStoreService

eee8983

update submodule

10e5912

lzxddz force-pushed the remove-mutex branch from f9cf553 to 10e5912 Compare October 14, 2025 07:23

lzxddz merged commit 42c52db into eloqdata:eloq-10.6.10 Oct 14, 2025
1 check passed

coderabbitai bot mentioned this pull request Nov 9, 2025

Support multi dss shards #158

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove mutex from DataStoreService#107

Remove mutex from DataStoreService#107
lzxddz merged 2 commits intoeloqdata:eloq-10.6.10from
lzxddz:remove-mutex

lzxddz commented Sep 7, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Sep 30, 2025 •

edited

Loading

Review failed

Review ran into problems

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Sep 30, 2025

Uh oh!

githubzilla left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

lzxddz commented Sep 7, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Review ran into problems

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

githubzilla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lzxddz commented Sep 7, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Sep 30, 2025 •

edited

Loading