You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "Maxwell-Guo (via GitHub)" <gi...@apache.org> on 2023/06/05 02:33:33 UTC
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2333: CASSANDRA-18305: Enhance nodetool compactionstats with existing MBean metrics
Maxwell-Guo commented on code in PR #2333:
URL: https://github.com/apache/cassandra/pull/2333#discussion_r1217364611
##########
src/java/org/apache/cassandra/tools/nodetool/CompactionStats.java:
##########
@@ -53,29 +58,82 @@ public class CompactionStats extends NodeToolCmd
public void execute(NodeProbe probe)
{
PrintStream out = probe.output().out;
+ out.print(pendingTasksAndConcurrentCompactorsStats(probe));
+ out.print(compactionsCompletedStats(probe));
+ out.print(compactionThroughPutStats(probe));
+ out.println();
CompactionManagerMBean cm = probe.getCompactionManagerProxy();
+ reportCompactionTable(cm.getCompactions(), probe.getCompactionThroughputBytes(), humanReadable, vtableOutput, out);
+ }
+
+ private static String pendingTasksAndConcurrentCompactorsStats(NodeProbe probe)
+ {
Map<String, Map<String, Integer>> pendingTaskNumberByTable =
(Map<String, Map<String, Integer>>) probe.getCompactionMetric("PendingTasksByTableName");
- int numTotalPendingTask = 0;
+ StringBuffer toPrint = new StringBuffer();
+ toPrint.append(String.format("%s concurrent compactors, %s pending tasks", probe.getConcurrentCompactors()
+ , numPendingTasks(pendingTaskNumberByTable)));
+ toPrint.append(LINE_SEPARATOR);
for (Entry<String, Map<String, Integer>> ksEntry : pendingTaskNumberByTable.entrySet())
{
+ String ksName = ksEntry.getKey();
for (Entry<String, Integer> tableEntry : ksEntry.getValue().entrySet())
- numTotalPendingTask += tableEntry.getValue();
+ {
+ toPrint.append("- " + ksName + '.' + tableEntry.getKey() + ": " + tableEntry.getValue());
Review Comment:
Can we use String.format uniformly.
Although its performance is not very good, and actually I prefer to use string concatenation.
But Cassandra seems to be all String.formt,So it seems that unity looks better
##########
src/java/org/apache/cassandra/tools/nodetool/CompactionStats.java:
##########
@@ -53,29 +58,82 @@ public class CompactionStats extends NodeToolCmd
public void execute(NodeProbe probe)
{
PrintStream out = probe.output().out;
+ out.print(pendingTasksAndConcurrentCompactorsStats(probe));
+ out.print(compactionsCompletedStats(probe));
+ out.print(compactionThroughPutStats(probe));
+ out.println();
CompactionManagerMBean cm = probe.getCompactionManagerProxy();
+ reportCompactionTable(cm.getCompactions(), probe.getCompactionThroughputBytes(), humanReadable, vtableOutput, out);
+ }
+
+ private static String pendingTasksAndConcurrentCompactorsStats(NodeProbe probe)
+ {
Map<String, Map<String, Integer>> pendingTaskNumberByTable =
(Map<String, Map<String, Integer>>) probe.getCompactionMetric("PendingTasksByTableName");
- int numTotalPendingTask = 0;
+ StringBuffer toPrint = new StringBuffer();
+ toPrint.append(String.format("%s concurrent compactors, %s pending tasks", probe.getConcurrentCompactors()
+ , numPendingTasks(pendingTaskNumberByTable)));
+ toPrint.append(LINE_SEPARATOR);
for (Entry<String, Map<String, Integer>> ksEntry : pendingTaskNumberByTable.entrySet())
{
+ String ksName = ksEntry.getKey();
for (Entry<String, Integer> tableEntry : ksEntry.getValue().entrySet())
- numTotalPendingTask += tableEntry.getValue();
+ {
+ toPrint.append("- " + ksName + '.' + tableEntry.getKey() + ": " + tableEntry.getValue());
+ toPrint.append(LINE_SEPARATOR);
+ }
}
- out.println("pending tasks: " + numTotalPendingTask);
+
+ return toPrint.toString();
+ }
+
+ private static int numPendingTasks(Map<String, Map<String, Integer>> pendingTaskNumberByTable)
+ {
+ int numTotalPendingTasks = 0;
for (Entry<String, Map<String, Integer>> ksEntry : pendingTaskNumberByTable.entrySet())
{
- String ksName = ksEntry.getKey();
for (Entry<String, Integer> tableEntry : ksEntry.getValue().entrySet())
- {
- String tableName = tableEntry.getKey();
- int pendingTaskCount = tableEntry.getValue();
+ numTotalPendingTasks += tableEntry.getValue();
+ }
- out.println("- " + ksName + '.' + tableName + ": " + pendingTaskCount);
- }
+ return numTotalPendingTasks;
+ }
+
+ private static String compactionsCompletedStats(NodeProbe probe)
+ {
+ Long completedTasks = (Long)probe.getCompactionMetric("CompletedTasks");
+ CassandraMetricsRegistry.JmxMeterMBean totalCompactionsCompletedMetrics =
+ (CassandraMetricsRegistry.JmxMeterMBean)probe.getCompactionMetric("TotalCompactionsCompleted");
+ NumberFormat formatter = new DecimalFormat("##.00");
+ StringBuffer toPrint = new StringBuffer();
+ toPrint.append(String.format("compactions completed: %s", completedTasks));
+ toPrint.append(LINE_SEPARATOR);
+ toPrint.append(String.format("\tminute rate: %s/second", formatter.format(totalCompactionsCompletedMetrics.getOneMinuteRate())));
+ toPrint.append(LINE_SEPARATOR);
+ toPrint.append(String.format("\t5 minute rate: %s/second", formatter.format(totalCompactionsCompletedMetrics.getFiveMinuteRate())));
+ toPrint.append(LINE_SEPARATOR);
+ toPrint.append(String.format("\t15 minute rate: %s/second", formatter.format(totalCompactionsCompletedMetrics.getFifteenMinuteRate())));
+ toPrint.append(LINE_SEPARATOR);
+ toPrint.append(String.format("\tMean rate: %s/second", formatter.format(totalCompactionsCompletedMetrics.getMeanRate())));
+ toPrint.append(LINE_SEPARATOR);
+
+ return toPrint.toString();
+ }
+
+ private static String compactionThroughPutStats(NodeProbe probe)
+ {
+ double configured = probe.getCompactionThroughputMebibytesAsDouble();
+ double actual = probe.getCompactionRate() / (1024 * 1024);
+ if(configured == 0)
+ {
+ return String.format("compaction throughput absolute: %s MBps", actual);
}
- out.println();
- reportCompactionTable(cm.getCompactions(), probe.getCompactionThroughputBytes(), humanReadable, vtableOutput, out);
+ else
+ {
+ double percentage = (actual / configured) * 100;
Review Comment:
I think I would change the code to :` double configured = probe.getCompactionThroughputMebibytesAsDouble();
if(configured == 0)
{
return String.format("compaction throughput absolute: %s MBps", actual);
}
else
{
double actual = probe.getCompactionRate() / (1024 * 1024);
double percentage = (actual / configured) * 100;
return String.format("compaction throughput ratio: %s MBps / %s MBps (%s%s)", actual, configured, percentage, "%");
}`
if configured is 0, there is no need to calculate the value of actual.
##########
test/unit/org/apache/cassandra/tools/nodetool/CompactionStatsTest.java:
##########
@@ -130,6 +130,14 @@ compactionId, OperationType.COMPACTION, CQLTester.KEYSPACE, currentTable(), byte
CompactionInfo.Unit.BYTES, (double) bytesCompacted / bytesTotal * 100);
Assertions.assertThat(stdout).containsPattern(expectedStatsPattern);
+ Assertions.assertThat(stdout).containsPattern("[0-9]* concurrent compactors, [0-9]* pending tasks");
Review Comment:
I think to be on the safe side, you can add a test case that uses the invokeNodetool method for full-link testing.
Though when using invokeNodetool from a client side , It is impossible to specifically determine the value of the test, but we can test and output some expected results, such as whether the output information has the print out field we need, and when there is no data, and when we write a certain amount of data then do a major, These print out data should met our expectations。
@driftx WDYT?
##########
src/java/org/apache/cassandra/tools/nodetool/CompactionStats.java:
##########
@@ -53,29 +58,82 @@ public class CompactionStats extends NodeToolCmd
public void execute(NodeProbe probe)
{
PrintStream out = probe.output().out;
+ out.print(pendingTasksAndConcurrentCompactorsStats(probe));
+ out.print(compactionsCompletedStats(probe));
+ out.print(compactionThroughPutStats(probe));
+ out.println();
CompactionManagerMBean cm = probe.getCompactionManagerProxy();
+ reportCompactionTable(cm.getCompactions(), probe.getCompactionThroughputBytes(), humanReadable, vtableOutput, out);
+ }
+
+ private static String pendingTasksAndConcurrentCompactorsStats(NodeProbe probe)
+ {
Map<String, Map<String, Integer>> pendingTaskNumberByTable =
(Map<String, Map<String, Integer>>) probe.getCompactionMetric("PendingTasksByTableName");
- int numTotalPendingTask = 0;
+ StringBuffer toPrint = new StringBuffer();
+ toPrint.append(String.format("%s concurrent compactors, %s pending tasks", probe.getConcurrentCompactors()
+ , numPendingTasks(pendingTaskNumberByTable)));
Review Comment:
I think there is a problem of code format
line 75 should have some space before ',' , so that In this way “，” can be aligned with "%$"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org