Set create_if_missing as true if store_handler is non-shared

[githubzilla]

Summary by CodeRabbit

• Bug Fixes
  • Improved consistency of data storage initialization during startup.
  • Added validation to prevent configuration errors in multi-node deployments, ensuring required components are present before initialization proceeds.
  • Refined data service startup logic for more reliable initialization and better error handling.

[coderabbitai]

Walkthrough

Modified initialization and validation logic in redis_service.cpp for RocksDB and EloqDS data store services. RocksDB handler now always receives create_if_missing=true. EloqDS path adds multi-node validation requiring peer node configuration. Service startup semantics adjusted to always pass true for RocksDB-based DSS initialization.

Changes

Cohort / File(s)	Summary
RocksDB initialization src/redis_service.cpp	RocksDB handler now consistently receives create_if_missing=true via constant instead of conditional expression
EloqDS multi-node validation src/redis_service.cpp	Added validation check: if ng_configs.size() > 1, logs error and aborts initialization with false, requiring eloq_dss_peer_node for multi-node configurations
EloqDS cluster configuration and DSS startup src/redis_service.cpp	Adjusted initialization flow post-DSS cluster config: retained dynamic cluster generation, replaced legacy config-load block; StartService invocation now always passes true for RocksDB paths instead of conditional bootstrap/single-node check

Sequence Diagram

sequenceDiagram
    participant Init as Initialization Flow
    participant Val as Multi-Node Validator
    participant RDB as RocksDB Handler
    participant DSS as DataStoreService
    
    Init->>Val: Check node group config
    alt Multiple node groups (ng_configs > 1)
        Val->>Val: Log error
        Val-->>Init: Return false (abort)
    else Single node or bootstrap
        Val-->>Init: Proceed
        Init->>RDB: Initialize with create_if_missing=true
        RDB-->>Init: Handler ready
        Init->>DSS: Configure cluster topology
        Init->>DSS: Call StartService(true)
        DSS-->>Init: Service started
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Focus areas: Control-flow logic changes in critical DSS initialization path; multi-node validation semantics and impact on startup conditions
• Interconnected changes: RocksDB handler behavior, EloqDS validation, and StartService invocation are logically coupled and require unified understanding
• Verify: Ensure create_if_missing=true unconditionally passing does not break bootstrap or single-node scenarios; validate multi-node guard conditions are correct and peer-node handling is enforced consistently

Possibly related PRs

• Remove mutex from DataStoreService #140: Introduces EloqDS::DataStoreService::StartService(bool) API that this PR calls with true for RocksDB paths
• Update cluster config #256: Modifies DSS cluster initialization, in-memory topology generation, and peer-node handling in the same initialization path

Suggested labels

trigger-ci

Suggested reviewers

• liunyl

Poem

🐰 Constants now reign where conditionals played,
RocksDB creation's shield firmly laid,
Multi-node whispers require a peer's voice,
DSS starts bold—true's the choice!
One rabbit hops through initialization's dance. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
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 (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately captures the primary change: setting create_if_missing to true for non-shared storage handlers, which aligns with the main focus of the changeset.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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: 0

🧹 Nitpick comments (1)

src/redis_service.cpp (1)

1009-1015: OK to force create_if_missing for local RocksDB; please fix comment typos.

Logic matches the PR intent. Nit: clean up the comment (spelling/grammar).

-            true,  // for non shared storage, always pass
-                   // create_if_missing=true, since no confilcts will happen
-                   // under
+            true,  // non-shared storage: always pass create_if_missing=true
+                   // No conflicts are expected in multi-node deployments.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between da776d0 and eef2905.

📒 Files selected for processing (1)

• src/redis_service.cpp (3 hunks)

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: githubzilla
Repo: eloqdata/eloqkv PR: 193
File: build_log_service.cmake:103-118
Timestamp: 2025-09-24T09:00:44.250Z
Learning: The build_log_service.cmake file is specifically for building the log_service submodule which only supports ROCKSDB log state, unlike build_eloq_log_service.cmake which handles multiple WITH_LOG_STATE options (MEMORY, ROCKSDB, ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS). Therefore, requiring RocksDB unconditionally in build_log_service.cmake is the correct behavior.

Learnt from: githubzilla
Repo: eloqdata/eloqkv PR: 193
File: concourse/scripts/build_tarball_open.bash:121-121
Timestamp: 2025-09-26T05:07:45.185Z
Learning: The open log service in build_tarball_open.bash only supports ROCKSDB as the single log state option, unlike the main eloq log service which supports multiple WITH_LOG_STATE options. Therefore, no WITH_LOG_STATE configuration flag is needed for the open log service build.

📚 Learning: 2025-09-24T09:00:44.250Z

Learnt from: githubzilla
Repo: eloqdata/eloqkv PR: 193
File: build_log_service.cmake:103-118
Timestamp: 2025-09-24T09:00:44.250Z
Learning: The build_log_service.cmake file is specifically for building the log_service submodule which only supports ROCKSDB log state, unlike build_eloq_log_service.cmake which handles multiple WITH_LOG_STATE options (MEMORY, ROCKSDB, ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS). Therefore, requiring RocksDB unconditionally in build_log_service.cmake is the correct behavior.

Applied to files:

• src/redis_service.cpp

📚 Learning: 2025-09-26T05:07:45.185Z

Learnt from: githubzilla
Repo: eloqdata/eloqkv PR: 193
File: concourse/scripts/build_tarball_open.bash:121-121
Timestamp: 2025-09-26T05:07:45.185Z
Learning: The open log service in build_tarball_open.bash only supports ROCKSDB as the single log state option, unlike the main eloq log service which supports multiple WITH_LOG_STATE options. Therefore, no WITH_LOG_STATE configuration flag is needed for the open log service build.

Applied to files:

• src/redis_service.cpp

📚 Learning: 2025-09-24T08:53:57.928Z

Learnt from: githubzilla
Repo: eloqdata/eloqkv PR: 193
File: src/redis_service.cpp:94-118
Timestamp: 2025-09-24T08:53:57.928Z
Learning: The LOG_STATE_TYPE_RKDB_CLOUD macro gating in src/redis_service.cpp includes `&& !defined(LOG_STATE_TYPE_RKDB)` condition intentionally to prevent conflicts when LOG_STATE_TYPE_RKDB is defined unconditionally in some builds (build_log_service.cmake) alongside cloud variants. The CMake configuration in build_eloq_log_service.cmake ensures mutual exclusivity of log state types.

Applied to files:

• src/redis_service.cpp

🔇 Additional comments (2)

src/redis_service.cpp (2)

1107-1119: Verify DSS snapshot/WAL behavior with unconditional create_if_missing=true in RocksDB path.

The approach is sound for non-shared storage; confirm that unconditionally creating the database does not impede snapshot or WAL catch-up on rejoining nodes. The optional runtime gflag override remains a reasonable enhancement for edge deployments:

DEFINE_bool(eloq_dss_create_if_missing, true,
            "If true, DSS with non-shared RocksDB creates DB when missing");

---

1061-1066: Multi-node detection fix is logically sound but requires manual verification of intent.

Based on code analysis:

• Confirmed: ng_configs is unordered_map<uint32_t, vector<NodeConfig>> where is_candidate_ is a field on individual nodes
• Concern is valid: A single node group with multiple candidate nodes bypasses the current ng_configs.size() > 1 check, which only counts node groups
• Suggested fix syntax appears correct: The proposed iteration pattern for (const auto &pair : ng_configs) { for (const auto &n : pair.second) matches existing code patterns at lines 869 and 2072

However, I could not locate the complete NodeConfig struct definition in search results. Before applying the fix, please verify:

1. That single-group multi-node clusters with multiple candidate nodes are an actual supported scenario requiring the eloq_dss_peer_node parameter
2. That the fix correctly identifies all nodes that should be counted as part of the multi-node cluster detection