apache / apache/kvrocks

Introduce subcommand as a first-class command abstraction

Open
#3,517 2 comments 0 reactions 1 assignee Claimed by @geetanshjuneja View on GitHub
good first issue help wanted
Dominant language
C++
Stars
4.4k
Forks
658
Avg merge
1d 20h
Merged PRs (30d)
10

Description

### Search before asking

- [x] I had searched in the issues and found no similar framework-level issue. I found issues about specific subcommands or documentation, but not one about making subcommands a first-class command abstraction.

### Motivation

Kvrocks has many Redis-compatible command families whose behavior is selected by the second token, such as `COMMAND GETKEYS`, `CONFIG GET/SET`, `CLIENT LIST/KILL`, `CLUSTER *`, `SCRIPT *`, and `NAMESPACE GET/SET/ADD/DEL/CURRENT`.

Currently, these subcommands are usually handled inside the root command implementation by manually parsing `args_[1]`. This makes subcommands second-class from the command framework's point of view:

- Subcommands cannot have their own registered metadata, such as arity, flags, key specs, and category.
- Command resolution, key extraction, command renaming, Lua command invocation, and command introspection have to reason about only the root command.
- Root command implementations tend to become large dispatchers instead of focused command handlers.
- It is harder to keep compatibility behavior consistent when different command families implement their own subcommand parsing rules.

A previous implementation attempt in #3413 demonstrated that subcommands can be represented explicitly in the command table and used by command lookup paths. It would be useful to formalize this direction as an enhancement.

### Solution

Introduce subcommand as a first-class abstraction in the command framework.

A possible implementation could include:

- Add subcommand registration APIs, for example `MakeSubCmdAttr(parent, subcommand, ...)`.
- Store subcommand metadata separately from root command metadata while keeping a canonical internal name such as `namespace|get`.
- Add command resolution based on the full command token vector, for example resolving `{"namespace", "get", "ns"}` to the `namespace|get` attributes while preserving `namespace` as the root command.
- Update command creation and lookup paths to use the resolved command attributes:
- client command execution
- Lua scripting command invocation
- `COMMAND GETKEYS`
- command renaming paths
- Allow each subcommand to define its own arity, flags, key range, and handler class.
- Refactor one existing command family, such as `NAMESPACE`, as the initial reference implementation.
- Preserve existing externally visible behavior, including legacy error messages for unknown or invalid subcommands.

The implementation in #3413 can be used as a reference. In particular, it covered:

- `CommandTable::Resolve(...)`
- subcommand registration and metadata lookup
- dedicated `NAMESPACE` subcommand handlers
- renamed root command resolution
- key extraction for registered subcommands
- C++ and Go regression tests

### Non-goals

- This should not change public command behavior unless required for correctness.
- This should not add new Redis commands by itself.
- This should not force all command families to migrate in a single PR. It is reasonable to introduce the framework first and migrate command families incrementally.

### Suggested tests

- C++ unit tests for root command resolution, registered subcommand resolution, unknown subcommand fallback, arity checks, key range extraction, and renamed root command behavior.
- Go integration tests for externally visible behavior, especially:
- `COMMAND GETKEYS` with subcommands
- command renaming with subcommands
- `NAMESPACE` legacy error behavior
- Lua invocation of commands with subcommands

### Are you willing to submit a PR?

- [ ] I'm willing to submit a PR!

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.