From 0c7410a9da1281dd4f7a70a0668903ebb61bda58 Mon Sep 17 00:00:00 2001 From: maxwellguo Date: Wed, 25 Oct 2023 08:25:42 +0200 Subject: [PATCH] Fix nodetool tablehistograms output to avoid printing repeated information and ensure at most two arguments patch by Maxwell Guo; reviewed by Stefan Miklosovic and Brandon Williams for CASSANDRA-18955 --- CHANGES.txt | 1 + .../tools/nodetool/TableHistograms.java | 23 ++- .../tools/nodetool/TableHistogramsTest.java | 152 ++++++++++++++++++ 3 files changed, 170 insertions(+), 6 deletions(-) create mode 100644 test/unit/org/apache/cassandra/tools/nodetool/TableHistogramsTest.java diff --git a/CHANGES.txt b/CHANGES.txt index 791d7e110a0c..517a20cdb8b3 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 5.0-alpha2 + * Fix nodetool tablehistograms output to avoid printing repeated information and ensure at most two arguments (CASSANDRA-18955) * Change the checksum algorithm SAI-related files use from CRC32 to CRC32C (CASSANDRA-18836) * Correctly remove Index.Group from IndexRegistry (CASSANDRA-18905) * Fix vector type to support DDM's mask_default function (CASSANDRA-18889) diff --git a/src/java/org/apache/cassandra/tools/nodetool/TableHistograms.java b/src/java/org/apache/cassandra/tools/nodetool/TableHistograms.java index fec47a62ea1a..08806a07f5e9 100644 --- a/src/java/org/apache/cassandra/tools/nodetool/TableHistograms.java +++ b/src/java/org/apache/cassandra/tools/nodetool/TableHistograms.java @@ -37,6 +37,7 @@ import org.apache.cassandra.tools.NodeTool.NodeToolCmd; import org.apache.cassandra.utils.EstimatedHistogram; +import org.apache.cassandra.utils.Pair; import org.apache.commons.lang3.ArrayUtils; @Command(name = "tablehistograms", description = "Print statistic histograms for a given table") @@ -60,21 +61,24 @@ public void execute(NodeProbe probe) allTables.put(entry.getKey(), entry.getValue().getTableName()); } - if (args.size() == 2) + if (args.size() == 2 && args.stream().noneMatch(arg -> arg.contains("."))) { tablesList.put(args.get(0), args.get(1)); } else if (args.size() == 1) { - String[] input = args.get(0).split("\\."); - checkArgument(input.length == 2, "tablehistograms requires keyspace and table name arguments"); - tablesList.put(input[0], input[1]); + Pair ksTbPair = parseTheKsTbPair(args.get(0)); + tablesList.put(ksTbPair.left, ksTbPair.right); } - else + else if (args.size() == 0) { // use all tables tablesList = allTables; } + else + { + throw new IllegalArgumentException("tablehistograms requires or format argument."); + } // verify that all tables to list exist for (String keyspace : tablesList.keys()) @@ -86,7 +90,7 @@ else if (args.size() == 1) } } - for (String keyspace : tablesList.keys()) + for (String keyspace : tablesList.keys().elementSet()) { for (String table : tablesList.get(keyspace)) { @@ -171,4 +175,11 @@ else if (args.size() == 1) } } } + + private Pair parseTheKsTbPair(String ksAndTb) + { + String[] input = args.get(0).split("\\."); + checkArgument(input.length == 2, "tablehistograms requires keyspace and table name arguments"); + return Pair.create(input[0], input[1]); + } } diff --git a/test/unit/org/apache/cassandra/tools/nodetool/TableHistogramsTest.java b/test/unit/org/apache/cassandra/tools/nodetool/TableHistogramsTest.java new file mode 100644 index 000000000000..733b876c092c --- /dev/null +++ b/test/unit/org/apache/cassandra/tools/nodetool/TableHistogramsTest.java @@ -0,0 +1,152 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.cassandra.tools.nodetool; + +import org.apache.cassandra.auth.AuthKeyspace; +import org.apache.cassandra.db.SystemKeyspace; +import org.apache.cassandra.schema.SchemaConstants; +import org.apache.cassandra.schema.SchemaKeyspace; +import org.apache.cassandra.schema.SystemDistributedKeyspace; +import org.apache.cassandra.tracing.TraceKeyspace; + +import org.apache.commons.lang3.StringUtils; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.cassandra.cql3.CQLTester; +import org.apache.cassandra.tools.ToolRunner; + +import static org.apache.cassandra.tools.ToolRunner.invokeNodetool; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; + +public class TableHistogramsTest extends CQLTester +{ + private static final String INFO_ROW = "Percentile Read Latency Write Latency SSTables Partition Size Cell Count"; + private final int ALL_TABLE_SIZE = SystemKeyspace.TABLE_NAMES.size() + + SchemaKeyspace.metadata().tables.size() + + TraceKeyspace.TABLE_NAMES.size() + + AuthKeyspace.TABLE_NAMES.size() + + SystemDistributedKeyspace.TABLE_NAMES.size(); + + @BeforeClass + public static void setup() throws Exception + { + requireNetwork(); + startJMXServer(); + } + + @Test + @SuppressWarnings("SingleCharacterStringConcatenation") + public void testMaybeChangeDocs() + { + // If you added, modified options or help, please update docs if necessary + ToolRunner.ToolResult tool = invokeNodetool("help", "tablehistograms"); + assertEquals(0, tool.getExitCode()); + tool.assertOnCleanExit(); + + String help = "NAME\n" + + " nodetool tablehistograms - Print statistic histograms for a given table\n" + + "\n" + + "SYNOPSIS\n" + + " nodetool [(-h | --host )] [(-p | --port )]\n" + + " [(-pp | --print-port)] [(-pw | --password )]\n" + + " [(-pwf | --password-file )]\n" + + " [(-u | --username )] tablehistograms [--]\n" + + " [
| ]\n" + + "\n" + + "OPTIONS\n" + + " -h , --host \n" + + " Node hostname or ip address\n" + + "\n" + + " -p , --port \n" + + " Remote jmx agent port number\n" + + "\n" + + " -pp, --print-port\n" + + " Operate in 4.0 mode with hosts disambiguated by port number\n" + + "\n" + + " -pw , --password \n" + + " Remote jmx agent password\n" + + "\n" + + " -pwf , --password-file \n" + + " Path to the JMX password file\n" + + "\n" + + " -u , --username \n" + + " Remote jmx agent username\n" + + "\n" + + " --\n" + + " This option can be used to separate command-line options from the\n" + + " list of argument, (useful when arguments might be mistaken for\n" + + " command-line options\n" + + "\n" + + " [
| ]\n" + + " The keyspace and table name\n" + + "\n" + + "\n"; + assertThat(tool.getStdout()).isEqualTo(help); + } + + @Test + public void testWithNoTableSpecified() + { + ToolRunner.ToolResult tool = invokeNodetool("tablehistograms"); + tool.assertOnCleanExit(); + assertThat(tool.getStdout()).contains(SchemaConstants.SYSTEM_KEYSPACE_NAME); + assertThat(tool.getStdout()).contains(SchemaConstants.SCHEMA_KEYSPACE_NAME); + assertThat(tool.getStdout()).contains(SchemaConstants.TRACE_KEYSPACE_NAME); + assertThat(tool.getStdout()).contains(SchemaConstants.AUTH_KEYSPACE_NAME); + assertThat(tool.getStdout()).contains(SchemaConstants.DISTRIBUTED_KEYSPACE_NAME); + assertThat(StringUtils.countMatches(tool.getStdout(), INFO_ROW)).isEqualTo(ALL_TABLE_SIZE); + } + + @Test + public void testWithOneTableSpecified() + { + //format 1 : ks.table + ToolRunner.ToolResult tool = invokeNodetool("tablehistograms", "system.local"); + tool.assertOnCleanExit(); + assertThat(tool.getStdout()).contains(SchemaConstants.SYSTEM_KEYSPACE_NAME); + assertThat(StringUtils.countMatches(tool.getStdout(), INFO_ROW)).isEqualTo(1); + + // format 2 : ks table + tool = invokeNodetool("tablehistograms", "system", "local"); + tool.assertOnCleanExit(); + assertThat(tool.getStdout()).contains(SchemaConstants.SYSTEM_KEYSPACE_NAME); + assertThat(StringUtils.countMatches(tool.getStdout(), INFO_ROW)).isEqualTo(1); + } + + @Test + public void testWithMoreThanOneTableSpecified() + { + //format 1 : ks1.tb1 ks2.tb2 + ToolRunner.ToolResult tool = invokeNodetool("tablehistograms", "system.local", "system.paxos"); + assertNotEquals(0, tool.getExitCode()); + assertThat(tool.getStdout()).contains("nodetool: tablehistograms requires
or format argument"); + + // format 2 : ks1 tb1 ks2 tb2 + tool = invokeNodetool("tablehistograms", "system", "local", "system", "paxos"); + assertNotEquals(0, tool.getExitCode()); + assertThat(tool.getStdout()).contains("nodetool: tablehistograms requires
or format argument"); + + // format 3 : ks1.tb1 ks2 + tool = invokeNodetool("tablehistograms", "system.local", "system"); + assertNotEquals(0, tool.getExitCode()); + assertThat(tool.getStdout()).contains("nodetool: tablehistograms requires
or format argument"); + } +}