Start data substrate when store_handler is nullptr only if data store is enabled#309
Start data substrate when store_handler is nullptr only if data store is enabled#309githubzilla merged 4 commits intomainfrom
Conversation
WalkthroughAdds a config-driven guard to DataStoreHandler validation in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/redis_service.cpp (1)
192-196: Init now correctly skips DataStoreHandler null-check when store is disabledThe new
enable_storeguard makesInitsucceed whenenable_data_storeis false even ifstore_hd_is null, which matches the laterskip_kv_ = !enable_data_storebehavior. This looks correct and should fix the previous failure mode when running with store disabled.If you want to reduce duplication, you could cache
GetCoreConfig()once and reuseenable_storeagain at line 230 when initializingskip_kv_, but that's purely optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/redis_service.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(0 hunks).pre-commit-config.yaml(1 hunks)include/redis_command.h(2 hunks)src/redis_service.cpp(3 hunks)
💤 Files with no reviewable changes (1)
- .gitignore
✅ Files skipped from review due to trivial changes (1)
- include/redis_command.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/redis_service.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
🔇 Additional comments (1)
.pre-commit-config.yaml (1)
15-21: Well-structured clang-format integration.The clang-format hook is properly configured with sensible defaults:
- Targets the same C/C++ file extensions as the other hooks.
- Excludes third-party and generated code appropriately.
- The
--style=file --fallback-style=nonearguments ensure the hook respects a.clang-formatconfiguration file if present, otherwise disables fallback styling (enforcing explicit project configuration).- Verbose output aids in debugging formatting issues in CI/pre-commit runs.
| - id: trailing-whitespace | ||
| files: \.(cpp|cxx|cc|c|h|hpp|hxx|hh)$ | ||
| exclude: (^include/redis|^src/redis|^include/vector|^src/vector|^data_substrate/|^tests/|^lua|^fpconv) | ||
| - id: end-of-file-fixer | ||
| files: \.(cpp|cxx|cc|c|h|hpp|hxx|hh)$ | ||
| exclude: (^include/redis|^src/redis|^include/vector|^src/vector|^data_substrate/|^tests/|^lua|^fpconv) | ||
| - id: check-added-large-files | ||
|
|
||
| - repo: https://github.com/pre-commit/mirrors-clang-format | ||
| rev: v18.1.8 | ||
| hooks: | ||
| - id: clang-format | ||
| files: \.(cpp|cxx|cc|c|h|hpp|hxx|hh)$ | ||
| exclude: (^include/redis/|^src/redis/|^include/vector/|^src/vector/|^data_substrate/|^tests/|^lua/|^fpconv/) | ||
| args: [--style=file, --fallback-style=none, --verbose] |
There was a problem hiding this comment.
Inconsistent path patterns across hooks may cause mismatches.
The exclusion patterns are inconsistent between the first two hooks and the clang-format hook:
- Lines 9, 12: Use
^lua|^fpconv(no trailing slashes) - Line 20: Use
^lua/|^fpconv/(with trailing slashes)
This inconsistency could cause the trailing-whitespace and end-of-file-fixer hooks to behave differently from clang-format when matching excluded directories. Regex patterns without trailing slashes may match files or directories with those names anywhere in the path, whereas patterns with slashes explicitly target directory boundaries.
Standardize the exclusion patterns for consistency:
- - id: trailing-whitespace
- files: \.(cpp|cxx|cc|c|h|hpp|hxx|hh)$
- exclude: (^include/redis|^src/redis|^include/vector|^src/vector|^data_substrate/|^tests/|^lua|^fpconv)
- - id: end-of-file-fixer
- files: \.(cpp|cxx|cc|c|h|hpp|hxx|hh)$
- exclude: (^include/redis|^src/redis|^include/vector|^src/vector|^data_substrate/|^tests/|^lua|^fpconv)
+ - id: trailing-whitespace
+ files: \.(cpp|cxx|cc|c|h|hpp|hxx|hh)$
+ exclude: (^include/redis/|^src/redis/|^include/vector/|^src/vector/|^data_substrate/|^tests/|^lua/|^fpconv/)
+ - id: end-of-file-fixer
+ files: \.(cpp|cxx|cc|c|h|hpp|hxx|hh)$
+ exclude: (^include/redis/|^src/redis/|^include/vector/|^src/vector/|^data_substrate/|^tests/|^lua/|^fpconv/)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - id: trailing-whitespace | |
| files: \.(cpp|cxx|cc|c|h|hpp|hxx|hh)$ | |
| exclude: (^include/redis|^src/redis|^include/vector|^src/vector|^data_substrate/|^tests/|^lua|^fpconv) | |
| - id: end-of-file-fixer | |
| files: \.(cpp|cxx|cc|c|h|hpp|hxx|hh)$ | |
| exclude: (^include/redis|^src/redis|^include/vector|^src/vector|^data_substrate/|^tests/|^lua|^fpconv) | |
| - id: check-added-large-files | |
| - repo: https://github.com/pre-commit/mirrors-clang-format | |
| rev: v18.1.8 | |
| hooks: | |
| - id: clang-format | |
| files: \.(cpp|cxx|cc|c|h|hpp|hxx|hh)$ | |
| exclude: (^include/redis/|^src/redis/|^include/vector/|^src/vector/|^data_substrate/|^tests/|^lua/|^fpconv/) | |
| args: [--style=file, --fallback-style=none, --verbose] | |
| - id: trailing-whitespace | |
| files: \.(cpp|cxx|cc|c|h|hpp|hxx|hh)$ | |
| exclude: (^include/redis/|^src/redis/|^include/vector/|^src/vector/|^data_substrate/|^tests/|^lua/|^fpconv/) | |
| - id: end-of-file-fixer | |
| files: \.(cpp|cxx|cc|c|h|hpp|hxx|hh)$ | |
| exclude: (^include/redis/|^src/redis/|^include/vector/|^src/vector/|^data_substrate/|^tests/|^lua/|^fpconv/) | |
| - id: check-added-large-files | |
| - repo: https://github.com/pre-commit/mirrors-clang-format | |
| rev: v18.1.8 | |
| hooks: | |
| - id: clang-format | |
| files: \.(cpp|cxx|cc|c|h|hpp|hxx|hh)$ | |
| exclude: (^include/redis/|^src/redis/|^include/vector/|^src/vector/|^data_substrate/|^tests/|^lua/|^fpconv/) | |
| args: [--style=file, --fallback-style=none, --verbose] |
🤖 Prompt for AI Agents
In .pre-commit-config.yaml around lines 7 to 21, the exclude regexes are
inconsistent (some entries use ^lua|^fpconv without trailing slashes while
clang-format uses ^lua/|^fpconv/), which can cause different matching behavior
across hooks; standardize the exclusion patterns across all hooks by making them
the same form (for example, append trailing slashes to the patterns on lines 9
and 12 to match the clang-format style: use ^lua/ and ^fpconv/), and ensure any
other hook exclude lines are updated to the same normalized regex so all hooks
behave consistently.
2 participants
… is enabled
Summary by CodeRabbit
Bug Fixes
Chores
Style
✏️ Tip: You can customize this high-level summary in your review settings.