Repository navigation
optimize and fix info command, add compact command - #476
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCaches OS/executable/memory at RedisService startup, exposes getters, refactors INFO to use cached values plus /proc VmRSS parsing, conditionally adds a COMPACT command and handler guarded by ELOQKV_WITH_DSS_ROCKSDB_CLOUD, and updates CI and a submodule pointer. ChangesSystem Info Caching for INFO Command
COMPACT command (DSS RocksDB Cloud)
CI & Submodule
Sequence Diagram (COMPACT flow)sequenceDiagram
participant Client
participant RedisHandler as CompactCommandHandler
participant RedisService
participant DataStore as DataStoreHandler
Client->>RedisHandler: "compact" request (args)
RedisHandler->>RedisService: ParseCompactCommand + ExecuteCommand(CompactCommand)
RedisService->>DataStore: CompactStore()
DataStore-->>RedisService: success/failure
RedisService-->>RedisHandler: command result (success/failure)
RedisHandler-->>Client: REDIS reply (OK / error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/redis_command.cpp`:
- Around line 2102-2116: GetProcessVmRSSKb currently reads tokens in triplets
which breaks when non-numeric lines appear before VmRSS; change it to read
/proc/<pid>/status line-by-line (use std::getline on the opened file), check for
a line that starts with "VmRSS:", then parse that line with a std::istringstream
(or find the first number) to extract and return the numeric KB value; update
GetProcessVmRSSKb to return 0 on any parse/open failure and ensure you reference
the function name GetProcessVmRSSKb when making the change.
In `@src/redis_service.cpp`:
- Around line 558-580: Replace the shell-dependent ExecCommand calls used to set
os_info_ and total_system_memory_kb_ with direct system calls/parsing: call
uname(2) (or the uname() libc wrapper) to build os_info_ instead of
ExecCommand("uname -srm"), keep and tighten the readlink("/proc/self/exe", ...)
logic used to populate executable_path_ (ensure proper error handling and no
cached failure string), and obtain total_system_memory_kb_ via
sysconf(_SC_PHYS_PAGES)*page_size or by parsing /proc/meminfo's "MemTotal:" line
as a fallback rather than ExecCommand("free | grep Mem | awk '{print $2}'");
also avoid caching shell error output or zero by catching/handling errors and
leaving fields empty or using sensible defaults when the system calls fail.
- Around line 152-170: The code currently dereferences rst.rbegin() without
ensuring rst is non-empty; add guards to avoid undefined behavior by checking
rst.empty() before any use of *rst.rbegin() — update the loop condition that
appends a newline (replace the check "if (rst.size() > 0 && *rst.rbegin() !=
'\n')" with a test that first ensures !rst.empty() before reading the last char)
and also guard the final if that trims the trailing newline (check rst.empty()
and return rst immediately if empty, otherwise inspect the last character). Use
the existing variables (rst, line) and keep the same pclose(fp) and return logic
otherwise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7231402-f26c-4d3f-94f8-9d9e8115ba54
📒 Files selected for processing (5)
data_substrateinclude/redis_command.hinclude/redis_service.hsrc/redis_command.cppsrc/redis_service.cpp
…oqdata#477) * feat: checkout private deps at version-matched branch Derive branch name (rel_x_x_x_eloqkv) from tag_name for eloq_log_service and raft_host_manager clones in CI. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * remove unused test-build.yml * clean --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
68-70: 💤 Low valueOptimize: clone, checkout, then update submodules once.
Cloning with
--recurse-submodulesinitializes submodules for the default branch, but line 69 immediately checks out a different branch, then line 70 updates submodules again. This causes redundant submodule initialization.♻️ More efficient approach
- git clone --recurse-submodules git@github.com:eloqdata/eloq_log_service.git logservice_src + git clone git@github.com:eloqdata/eloq_log_service.git logservice_src git -C logservice_src checkout $REL_BRANCH git -C logservice_src submodule update --init --recursive🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build.yml around lines 68 - 70, The current workflow redundantly initializes submodules by using git clone --recurse-submodules before checking out $REL_BRANCH and then running git -C logservice_src submodule update --recursive again; change the sequence to plain git clone (remove --recurse-submodules), then git -C logservice_src checkout $REL_BRANCH, and finally run git -C logservice_src submodule update --init --recursive so submodules are initialized/updated only after the branch checkout (referencing the existing commands git clone, git -C ... checkout $REL_BRANCH, and git -C ... submodule update --recursive).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build.yml:
- Line 66: The REL_BRANCH assignment uses unvalidated interpolation of
inputs.tag_name, which risks shell injection; sanitize and quote the tag before
building REL_BRANCH: validate inputs.tag_name by stripping or replacing any
characters except a safe whitelist (e.g., [A-Za-z0-9._-]) and assign that
sanitized value to a temporary variable, then construct REL_BRANCH using the
sanitized variable and wrap it in double quotes to prevent
word-splitting/expansion; reference the REL_BRANCH assignment and the
inputs.tag_name expansion when making this change.
- Around line 69-70: Before running git -C logservice_src checkout $REL_BRANCH
and git -C raft_host_manager_src checkout $REL_BRANCH, validate that
origin/$REL_BRANCH exists; replicate the logic from scripts/git-checkout.sh by
running git ls-remote --heads origin "$REL_BRANCH" | grep -q "$REL_BRANCH" and
if found use git checkout -b "$REL_BRANCH" "origin/$REL_BRANCH", otherwise print
a clear error like "Expected release branch $REL_BRANCH not found on origin" and
exit 1; apply this change for both the logservice_src and raft_host_manager_src
checkout steps so the workflow fails fast when the remote release branch is
missing.
In `@include/redis_command.h`:
- Around line 279-281: The enum RedisCommandType must not insert COMPACT
mid-list because RedisCommandType is serialized as a raw uint8_t in Serialize()
implementations; move the COMPACT enumerator to the end of the RedisCommandType
enum (after FAILOVER and all existing values) or alternatively give every enum
member an explicit stable numeric value so IDs don't change across builds;
update any related comments to reflect the new ordering and ensure
Serialize()/Deserialize() continue to use the stable numeric IDs.
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 68-70: The current workflow redundantly initializes submodules by
using git clone --recurse-submodules before checking out $REL_BRANCH and then
running git -C logservice_src submodule update --recursive again; change the
sequence to plain git clone (remove --recurse-submodules), then git -C
logservice_src checkout $REL_BRANCH, and finally run git -C logservice_src
submodule update --init --recursive so submodules are initialized/updated only
after the branch checkout (referencing the existing commands git clone, git -C
... checkout $REL_BRANCH, and git -C ... submodule update --recursive).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78f5ab39-9339-498d-add7-d92b753cf6a9
📒 Files selected for processing (8)
.github/workflows/build.yml.github/workflows/test-build.ymlinclude/redis_command.hinclude/redis_handler.hinclude/redis_service.hsrc/redis_command.cppsrc/redis_handler.cppsrc/redis_service.cpp
💤 Files with no reviewable changes (1)
- .github/workflows/test-build.yml
👮 Files not reviewed due to content moderation or server errors (1)
- src/redis_command.cpp
starrysky9959
approved these changes
Jun 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Chores