CASSANDRA-20793: Split nodetool compact into keyspace, sstables, and range subcommands#4922
CASSANDRA-20793: Split nodetool compact into keyspace, sstables, and range subcommands#4922arvindKandpal-ksolves wants to merge 1 commit into
Conversation
…range subcommands - Deprecated legacy compact command with warning message - Added compact keyspace subcommand for keyspace/table compaction - Added compact sstables subcommand for user-defined SSTable compaction - Added compact range subcommand for token range compaction - Added unit tests in CompactMockTest and CompactTest
| * @deprecated See CASSANDRA-20793. Use {@code compact keyspace}, {@code compact sstables}, | ||
| * or {@code compact range} instead. | ||
| */ | ||
| @Deprecated(since = "7.0") |
There was a problem hiding this comment.
I've set the deprecated version to 7.0 for now. Could you please confirm if this is the correct version or if it should be 5.x?
| @@ -66,6 +74,8 @@ public class Compact extends AbstractCommand | |||
| @Override | |||
| public void execute(NodeProbe probe) | |||
| { | |||
| probe.output().out.println("WARNING: nodetool compact is deprecated, use 'compact keyspace', 'compact sstables', or 'compact range' instead."); | |||
|
|
|||
| final boolean startEndTokenProvided = !(startToken.isEmpty() && endToken.isEmpty()); | |||
| final boolean partitionKeyProvided = !partitionKey.isEmpty(); | |||
| final boolean tokenProvided = startEndTokenProvided || partitionKeyProvided; | |||
| @@ -118,4 +128,149 @@ else if (partitionKeyProvided) | |||
| } | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
I see a backward compatibility edge case here. If a user has a keyspace actually named keyspace or range, running the legacy command like nodetool compact keyspace mytable will trigger the new subcommand instead of treating it as the keyspace name. The same issue would happen if a file is named sstables.
Any suggestions on what's the standard way in the codebase to handle this kind of picocli name collision? Should I add some custom check before picocli parses the arguments?
|
I just tested this locally, and you are totally right about the collision. If a user actually has a keyspace named Here is what I reproduced: cqlsh> CREATE KEYSPACE "keyspace" WITH replication = {'class': 'SimpleStrategy', 'replication_factor': 1};
cqlsh> CREATE TABLE "keyspace".my_table (id int PRIMARY KEY);$ bin/nodetool compact keyspace my_table
nodetool: Keyspace [my_table] does not exist.Since the keyspace keyword gets swallowed as a subcommand, Picocli ends up treating my_table as the keyspace name. What is the preferred way to fix this CLI ambiguity in our codebase? Should I add a custom pre-parsing check (maybe in NodeTool.java) before passing the args to Picocli, so we can route these specific edge cases back to the legacy behavior? |
1 participant
Summary
Splits the
nodetool compactcommand into three distinct subcommands to comply with picocli best practices as per CASSANDRA-20793:compact keyspace- for keyspace/table compactioncompact sstables- for user-defined SSTable compactioncompact range- for token range compactionThe legacy
compactcommand is deprecated (with a printed warning) but kept fully intact for backward compatibility.Changes
Compact.java: Added 3 static inner subcommand classes (Keyspace,SSTables,Range). Deprecated the parent command and added a deprecation warning output.CompactTest.java: Added integration tests for all new subcommands and verified the legacy command warning.CompactMockTest.java: Added mock/unit tests for the new subcommands to ensure arguments are parsed and passed correctly.Tests
All existing tests pass. New test coverage has been added for all 3 subcommands to ensure backward compatibility and correct execution.
patch by Arvind Kandpal; reviewed by TBD for CASSANDRA-20793