feat(commands): introduce subcommand as a primary abstraction by jihuayu · Pull Request #3413 · apache/kvrocks

jihuayu · 2026-04-01T02:01:56Z

This PR introduces subcommand as a first-class abstraction.

Key changes include:

Supported standardized registration and parsing processes for subcommands.
Implemented metadata management for subcommand structures.
Refactored namespace as a reference implementation for this new abstraction.

AI Disclosure

The architectural design of this PR was completed independently. Some code segments were developed with AI.
All test cases in this PR were either autonomously generated by AI or generated based on natural language descriptions provided by me.

…ed commands

…ng key arguments

Copilot

Pull request overview

This PR introduces “subcommand” as a first-class command abstraction by adding subcommand registration/resolution to the command table, updating command lookup to resolve root+subcommand attributes, and refactoring NAMESPACE into separate subcommand implementations.

Changes:

Added subcommand registration and resolution via CommandTable::Resolve, plus supporting metadata structures.
Updated command creation paths (connection execution + Lua scripting) to resolve commands using full token vectors (enabling subcommand-aware dispatch and renamed-root support).
Refactored NAMESPACE into a root command that errors on invalid subcommands plus dedicated get/set/add/del/current subcommands, and added tests for subcommand resolution + legacy error behavior.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/gocase/unit/namespace/namespace_test.go	Adds regression coverage for legacy invalid-subcommand error messaging for `NAMESPACE`.
tests/gocase/unit/command/command_test.go	Adds coverage ensuring `COMMAND GETKEYS` works when the target root command is renamed.
tests/cppunit/subcommand_resolution_test.cc	Adds unit tests validating subcommand resolution, arity handling, key-range extraction, and renamed-root behavior.
src/storage/scripting.cc	Updates Lua command invocation to resolve commands using full tokens and use canonical root name for execution checks.
src/server/server.h	Changes command lookup API to accept full token vectors.
src/server/server.cc	Implements token-based command resolution/creation, setting canonical root name on the Commander instance.
src/server/redis_connection.cc	Updates connection execution path to resolve commands using full tokens and use canonical root name for downstream logic.
src/commands/commander.h	Introduces `ResolvedCommand`, subcommand maps, `MakeSubCmdAttr`, and the `Resolve` API; extends `Commander` with a stored root name.
src/commands/commander.cc	Implements subcommand registration, lookup, and `CommandTable::Resolve`; updates key extraction to use Commander parsing with set args/attrs.
src/commands/cmd_server.cc	Refactors `NAMESPACE` into root+subcommand implementations and registers `namespace

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-02T07:21:59Z

+StatusOr<ResolvedCommand> CommandTable::Resolve(const std::vector<std::string> &cmd_tokens) {
+  if (cmd_tokens.empty()) {
+    return {Status::RedisUnknownCmd};
+  }
+


COMMAND metadata APIs currently only consult commands/original_commands (root commands). With the new subcommand table (sub_commands/redis_subcommand_table) and Resolve returning subcommand attributes like namespace|add, subcommands won’t be discoverable via COMMAND, and COMMAND INFO namespace|add will return nil. Consider extending GetAllCommandsInfo / GetCommandsInfo / Size (and any other command-lookup metadata paths) to include registered subcommands as separate entries using the parent|sub name so clients can introspect them.

To avoid making this PR too large, I’ll handle this in a follow-up PR.

Copilot · 2026-04-02T07:21:59Z

+    MakeSubCmdAttr<CommandNamespaceSet>("namespace", "set", 4, "read-only admin skip-monitor", NO_KEY),
+    MakeSubCmdAttr<CommandNamespaceAdd>("namespace", "add", 4, "read-only admin skip-monitor", NO_KEY),
+    MakeSubCmdAttr<CommandNamespaceDel>("namespace", "del", 3, "read-only admin skip-monitor", NO_KEY),


These subcommands call Namespace::Set/Add/Del which can rewrite config and/or write to the Propagate CF, so they are not actually "read-only". Leaving them tagged as read-only means they’ll bypass Lua no-writes enforcement and other write-related safety checks. Update the flag strings for SET/ADD/DEL to include write (and remove read-only) so they’re treated as mutating operations.

Suggested change

MakeSubCmdAttr<CommandNamespaceSet>("namespace", "set", 4, "read-only admin skip-monitor", NO_KEY),

MakeSubCmdAttr<CommandNamespaceAdd>("namespace", "add", 4, "read-only admin skip-monitor", NO_KEY),

MakeSubCmdAttr<CommandNamespaceDel>("namespace", "del", 3, "read-only admin skip-monitor", NO_KEY),

MakeSubCmdAttr<CommandNamespaceSet>("namespace", "set", 4, "write admin skip-monitor", NO_KEY),

MakeSubCmdAttr<CommandNamespaceAdd>("namespace", "add", 4, "write admin skip-monitor", NO_KEY),

MakeSubCmdAttr<CommandNamespaceDel>("namespace", "del", 3, "write admin skip-monitor", NO_KEY),

@PragmaTwice I have the same question. However, the existing namespace command is read-only. Do we need to modify it?

Copilot · 2026-04-02T07:21:59Z

+// renamed root command GETKEYS
+func TestCommandGetKeysWithRenamedCommand(t *testing.T) {
+	srv := util.StartServer(t, map[string]string{
+		"rename-command MGET": "RENAMED_MGET",
+	})


The comment says "renamed root command GETKEYS", but this test is renaming the root command MGET and then calling the COMMAND GETKEYS subcommand. Consider updating the comment/test name to avoid confusion about what is being renamed.

jihuayu · 2026-04-03T03:14:39Z

Hi @git-hulk. I noticed that all Redis CLI-related test cases are failing on the Ubuntu platform, even though they pass just fine on my local Ubuntu machine. Any ideas on what might be causing this?

git-hulk · 2026-04-03T03:30:41Z

Hi @git-hulk. I noticed that all Redis CLI-related test cases are failing on the Ubuntu platform, even though they pass just fine on my local Ubuntu machine. Any ideas on what might be causing this?

It seems like a flaky test in Ubuntu 22.04, but I also cannot tell without diving deep.

Tangruilin · 2026-04-05T09:48:31Z

+    std::cout << fmt::format("Duplicate command registration for '{}'", attr.name) << std::endl;
+    std::abort();


"Are std::cout and std::abort used for debugging?"

This serves as a reminder for developers to avoid this error, as it can lead to unpredictable behavior.
In standard execution flow, this code should never be reached.

"Typically, for such cases, we should use FATAL logging and assert_debug for safeguards, rather than std::cout and std::abort. Here are two main reasons:

For a server service, we don't print std::cout to the terminal, so using std::cout vs. a logging library doesn't make a fundamental difference. Even if we want terminal output, I
think error messages should go to std::cerr rather than std::cout. (PS: I believe the best practice is to log at FATAL level.)

Although this is code that should never be reached, at the server level, nothing is worse than a crash. If this path is somehow triggered in production (meaning kvrocks has a bug),
the application shouldn't crash. A better approach would be to use assert_debug, which only crashes in debug mode. In release mode, the impact should be minimized.

What do you think?"

"However, it looks like this code path will only be reached during initialization. Both approaches seem acceptable."

Tangruilin · 2026-04-05T09:52:01Z

+    }
+    if (IsNamespaceCommandDisabled(srv)) {
+      return {Status::RedisExecErr, "namespace command is not allowed when redis-databases > 0"};
+    }


"Both IsNamespaceReadOnlyOnSlave and IsNamespaceCommandDisabled checks seem to be needed by all subcommands. Could we consider abstracting them into a common function?"

Namespace GET not need IsNamespaceReadOnlyOnSlave

Tangruilin · 2026-04-05T18:18:34Z

+  }
+
+  auto normalized_parent = util::ToLower(name.substr(0, delimiter));
+  auto normalized_sub = util::ToLower(name.substr(delimiter + 1));


The check here looks redundant. If delimiter + 1 >= name.size() is true, then name.substr(delimiter + 1) must be empty.

sonarqubecloud · 2026-04-06T11:27:57Z

Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
68.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

jihuayu added 4 commits April 1, 2026 15:25

feat(commands): add subcommand registration support

069221f

feat(commands): enhance subcommand resolution and add tests for renam…

9cf6a9a

…ed commands

fix(commands): improve lambda capture in GetKeysFromCommand for clarity

cca5fa8

fix(commands): improve error handling in GetKeysFromCommand for missi…

62f3cc1

…ng key arguments

jihuayu force-pushed the subkey branch from 9cfcdd4 to 62f3cc1 Compare April 1, 2026 07:25

Merge branch 'unstable' into subkey

b6cb68a

jihuayu changed the title ~~feat(commands): support registered subcommand resolution~~ feat(commands): introduce subcommand as a primary abstraction Apr 2, 2026

jihuayu marked this pull request as ready for review April 2, 2026 07:11

jihuayu requested review from PragmaTwice, Copilot and git-hulk and removed request for PragmaTwice and Copilot April 2, 2026 07:14

Copilot started reviewing on behalf of jihuayu April 2, 2026 07:15 View session

Copilot AI reviewed Apr 2, 2026

View reviewed changes

fix(ci): update redis build process to disable jemalloc

64b42d5

Tangruilin reviewed Apr 5, 2026

View reviewed changes

Merge branch 'unstable' into subkey

2ca0036

Tangruilin reviewed Apr 5, 2026

View reviewed changes

Merge branch 'unstable' into subkey

1c7c026

		std::cout << fmt::format("Duplicate command registration for '{}'", attr.name) << std::endl;
		std::abort();

Conversation

jihuayu commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Disclosure

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

jihuayu commented Apr 3, 2026

Uh oh!

git-hulk commented Apr 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud bot commented Apr 6, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jihuayu commented Apr 1, 2026 •

edited

Loading