Skip to content

refactor: move expensive info stats to standalone commands - #519

Merged
thweetkomputer merged 5 commits into
mainfrom
feature/keyspace-command
Jun 25, 2026
Merged

thweetkomputer merged 5 commits into
mainfrom
feature/keyspace-command

Conversation

@thweetkomputer

@thweetkomputer thweetkomputer commented Jun 24, 2026 •

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • New Features
    • Added support for the keyspace Redis command in cloud-enabled builds.
    • Added memory stats to report memory allocation/commit and the last checkpoint timestamp.
  • Bug Fixes
    • Improved memory command argument validation with clearer error replies for invalid subcommands/arity.
    • Updated INFO output to omit keyspace and moved memory/checkpoint fields, keeping response formatting consistent.

@coderabbitai

coderabbitai Bot commented Jun 24, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5166fd06-e2b4-48a7-9a39-6db7777d8f16

📥 Commits

Reviewing files that changed from the base of the PR and between 5a2a0d2 and e1fbf0b.

📒 Files selected for processing (1)
  • data_substrate
✅ Files skipped from review due to trivial changes (1)
  • data_substrate

Walkthrough

Adds standalone MemoryStatsCommand and DSS-cloud-gated KeyspaceCommand support, removes those fields from InfoCommand, and wires the new commands through parsing, execution, output formatting, handlers, and service registration.

Changes

KEYSPACE and MemoryStatsCommand feature

Layer / File(s) Summary
Command contracts
include/redis_command.h
Adds KEYSPACE = 186 and a matching static_assert under ELOQKV_WITH_DSS_ROCKSDB_CLOUD. Declares MemoryStatsCommand and KeyspaceCommand, and declares ParseKeyspaceCommand.
Handler declaration and registration
include/redis_handler.h, src/redis_service.cpp
Declares KeyspaceCommandHandler under the DSS-cloud guard and registers it for "keyspace" in RedisServiceImpl::AddHandlers().
Command execution paths
src/redis_command.cpp
Adds "keyspace" to command parsing, moves memory/checkpoint collection into MemoryStatsCommand::Execute, and adds KeyspaceCommand::Execute for DB sizes and disk key counts.
Response formatting
src/redis_command.cpp
Removes data_memory_allocated, data_memory_committed, and last_success_ckpt_ts from InfoCommand output, updates trailing CRLF formatting, and adds RESP output for MemoryStatsCommand and KeyspaceCommand.
Parsing and dispatch
src/redis_command.cpp
Routes KEYSPACE through ParseMultiCommand to ParseKeyspaceCommand, and validates the keyspace argument list in ParseKeyspaceCommand.
Run handlers
src/redis_handler.cpp
Adds KeyspaceCommandHandler::Run, changes memory argument validation, and adds the memory stats branch that executes MemoryStatsCommand.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • eloqdata/eloqkv#476: This PR overlaps on InfoCommand field changes and INFO output adjustments in the same DSS-cloud area.

Suggested reviewers

  • liunyl
  • starrysky9959
  • liangjchen

Poem

🐇 A keyspace hop, a stats-report tune,
Memory splits off beneath the moon.
INFO grows lean, then bows on cue,
New handlers wake and carry it through.
The rabbit twitches: “All neat, all bright —
fresh command paths now run just right.”

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.06% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main change: moving expensive INFO stats into standalone commands.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/keyspace-command

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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_handler.cpp`:
- Around line 4985-4988: The new arity-error paths in the MEMORY command handler
are missing the standard ERR prefix, so update the reply text in the MEMORY
parsing logic to match nearby Redis command handlers and exact client
expectations. In the MEMORY handling code, including the checks around the
command parser and arity validation paths, change the error messages to start
with ERR while preserving the existing command-specific wording so both affected
branches behave consistently.
🪄 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: 1c7bfe27-ee06-4ea3-b22f-54d84762a30d

📥 Commits

Reviewing files that changed from the base of the PR and between c9d28b5 and a57285c.

📒 Files selected for processing (5)
  • include/redis_command.h
  • include/redis_handler.h
  • src/redis_command.cpp
  • src/redis_handler.cpp
  • src/redis_service.cpp

Comment thread src/redis_handler.cpp

@zhangh43 zhangh43 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

@thweetkomputer
thweetkomputer merged commit a60ecc0 into main Jun 25, 2026
4 of 5 checks passed
@thweetkomputer
thweetkomputer deleted the feature/keyspace-command branch June 25, 2026 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants