Skip to content

remove extra setting of store worker num#261

Merged
thweetkomputer merged 2 commits into
mainfrom
remove-extra-params
Dec 4, 2025
Merged

remove extra setting of store worker num#261
thweetkomputer merged 2 commits into
mainfrom
remove-extra-params

Conversation

@thweetkomputer

@thweetkomputer thweetkomputer commented Dec 4, 2025

Copy link
Copy Markdown
Collaborator

Here are some reminders before you submit the pull request

  • Add tests for the change
  • Document changes
  • Reference the link of issue using fixes eloqdb/tx_service#issue_id
  • Reference the link of RFC if exists
  • Pass ./mtr --suite=mono_main,mono_multi,mono_basic

Summary by CodeRabbit

  • Style

    • Minor formatting adjustments in configuration-related code and metrics retrieval calls.
  • Chores

    • Updated conditional build-time logic to allow configuring worker count for a specific data store when that option is enabled.

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

@coderabbitai

coderabbitai Bot commented Dec 4, 2025

Copy link
Copy Markdown

Caution

Review failed

The pull request is closed.

Walkthrough

Formatting change in EngineConfig initializer, a minor formatting-only edit in metrics initialization call, and a conditional compile block added to set Eloq store worker count via gflags when DATA_STORE_TYPE_ELOQDSS_ELOQSTORE is defined.

Changes

Cohort / File(s) Summary
Header formatting
core/include/data_substrate.h
Cosmetic: reformatted EngineConfig member initializer (engine_registered) across multiple lines; no signature or behavioral change.
Eloq store worker config
core/src/data_substrate.cpp
Added #ifdef DATA_STORE_TYPE_ELOQDSS_ELOQSTORE block inside LoadCoreAndNetworkConfig() to set Eloq store worker count from core_num via gflags.
Metrics call formatting
core/src/metrics_init.cpp
Reflowed arguments/indentation of a GetBoolean call for enable_remote_request_metrics; no behavioral change.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Review core/src/data_substrate.cpp conditional compile guard and ensure core_num is the intended value for the Eloq worker count.
  • Confirm no unintended macro or build implications from the new #ifdef block.
  • Quick glance at core/include/data_substrate.h and core/src/metrics_init.cpp for formatting-only risks.

Poem

🐰 I hopped through code with gentle cheer,
Split a line, then changed one gear.
A guarded flag, a worker set,
Quiet tweaks—no larger threat.
Hop on, build, the logs are clear! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description contains only the unchecked template checklist with no actual content explaining the changes, rationale, or implementation details. Add substantive description explaining what 'extra setting' is being removed, why it's redundant, and provide context about the changes to LoadCoreAndNetworkConfig and the gflags conditional compilation.
Docstring Coverage ⚠️ Warning Docstring coverage is 50.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
Title check ✅ Passed The title 'remove extra setting of store worker num' directly describes the main change: removing redundant store worker number configuration in the codebase.

📜 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 760d6db and b61e14e.

📒 Files selected for processing (1)
  • core/src/metrics_init.cpp (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.

@githubzilla githubzilla left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

+1

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
core/src/data_substrate.cpp (1)

753-756: Guarded initialization of eloq_store_worker_num looks good; consider preserving CLI overrides

The new #ifdef DATA_STORE_TYPE_ELOQDSS_ELOQSTORE guard is a nice safety net. If you want to allow operators to override eloq_store_worker_num via CLI, you could gate the assignment on CheckCommandLineFlagIsDefault:

#ifdef DATA_STORE_TYPE_ELOQDSS_ELOQSTORE
-    GFLAGS_NAMESPACE::SetCommandLineOption(
-        "eloq_store_worker_num", std::to_string(core_config_.core_num).c_str());
+    if (CheckCommandLineFlagIsDefault("eloq_store_worker_num")) {
+        GFLAGS_NAMESPACE::SetCommandLineOption(
+            "eloq_store_worker_num",
+            std::to_string(core_config_.core_num).c_str());
+    }
#endif
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 213ff3a and 760d6db.

📒 Files selected for processing (2)
  • core/include/data_substrate.h (1 hunks)
  • core/src/data_substrate.cpp (1 hunks)
🔇 Additional comments (1)
core/include/data_substrate.h (1)

118-125: Formatting-only change to EngineConfig initializers is fine

Splitting engine_registered’s initializer across lines doesn’t affect behavior and keeps defaults (false) unchanged.

@thweetkomputer thweetkomputer merged commit 7f49b02 into main Dec 4, 2025
3 of 4 checks passed
@thweetkomputer thweetkomputer deleted the remove-extra-params branch December 4, 2025 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants