jyothsnakonisa commented on code in PR #4540:
URL: https://github.com/apache/cassandra/pull/4540#discussion_r2670370215
##########
src/java/org/apache/cassandra/db/compaction/CompactionTask.java:
##########
Review Comment:
Please remove unused imports in this file
##########
test/unit/org/apache/cassandra/db/compaction/CompactionTaskTest.java:
##########
@@ -103,6 +103,12 @@ public void testTaskIdIsPersistedInCompactionHistory()
TimeUUID persistedId = one.getTimeUUID("id");
Assert.assertEquals(id, persistedId);
+
+ String type = one.getString("compaction_type");
+ Assert.assertEquals("Compaction", type);
+
+ java.util.Map<String, String> properties =
one.getMap("compaction_properties",
org.apache.cassandra.db.marshal.UTF8Type.instance,
org.apache.cassandra.db.marshal.UTF8Type.instance);
Review Comment:
Please avoid fully qualified class names
```suggestion
Map<String, String> properties = one.getMap("compaction_properties",
UTF8Type.instance, UTF8Type.instance);
```
##########
src/java/org/apache/cassandra/db/compaction/CompactionHistoryTabularData.java:
##########
@@ -30,11 +30,13 @@
public class CompactionHistoryTabularData
{
private static final String[] ITEM_NAMES = new String[]{ "id",
"keyspace_name", "columnfamily_name", "compacted_at",
- "bytes_in",
"bytes_out", "rows_merged", "compaction_properties" };
+ "bytes_in",
"bytes_out", "rows_merged", "compaction_properties",
+ "compaction_type"
}; // Added
Review Comment:
Comment `// Added` might not be needed
##########
test/unit/org/apache/cassandra/db/compaction/CompactionTaskTest.java:
##########
@@ -71,7 +71,7 @@ public void setUp() throws Exception
cfs.truncateBlocking();
}
- @Test
+@Test
public void testTaskIdIsPersistedInCompactionHistory()
{
Review Comment:
Please fix the indentation
```suggestion
@Test
public void testTaskIdIsPersistedInCompactionHistory()
{
```
##########
src/java/org/apache/cassandra/tools/nodetool/CompactionHistory.java:
##########
@@ -33,10 +33,10 @@ public class CompactionHistory extends AbstractCommand
description = "Output format (json, yaml)")
private String outputFormat = "";
- @Option(paramLabel = "human_readable",
- names = { "-H", "--human-readable" },
- description = "Display bytes in human readable form, i.e. KiB,
MiB, GiB, TiB")
- private boolean humanReadable = false;
+ @Option(paramLabel = "no_human_readable",
+ names = { "-n", "--no-human-readable" },
+ description = "Display raw bytes (disable default human readable
format)")
+ private boolean noHumanReadable = false;
Review Comment:
I am concerned about changing the flag name and default behavior. Imagine
some scripts using this node tool command there is a chance of breaking them.
##########
src/java/org/apache/cassandra/db/compaction/CompactionHistoryTabularData.java:
##########
@@ -45,15 +47,16 @@ public class CompactionHistoryTabularData
private static final CompositeType COMPOSITE_TYPE;
private static final TabularType TABULAR_TYPE;
-
+
public static final String COMPACTION_TYPE_PROPERTY = "compaction_type";
-
- static
+
Review Comment:
Since Compaction Type property is another column in the table, you can get
rid of this constant.
Also, noticed that `CompactionHistorySystemTableUpgradeTest` has compilation
errors with these changes that you introduced in this patch.
Also, remove unused imports in files where this constant is used.
##########
src/java/org/apache/cassandra/db/compaction/CompactionHistoryTabularData.java:
##########
@@ -30,11 +30,13 @@
public class CompactionHistoryTabularData
{
private static final String[] ITEM_NAMES = new String[]{ "id",
"keyspace_name", "columnfamily_name", "compacted_at",
- "bytes_in",
"bytes_out", "rows_merged", "compaction_properties" };
+ "bytes_in",
"bytes_out", "rows_merged", "compaction_properties",
+ "compaction_type"
}; // Added
private static final String[] ITEM_DESCS = new String[]{ "time uuid",
"keyspace name",
"column family
name", "compaction finished at",
- "total bytes in",
"total bytes out", "total rows merged", "compaction properties" };
+ "total bytes in",
"total bytes out", "total rows merged",
+ "compaction
properties", "compaction type" }; // Added
Review Comment:
Comment `// Added` might not be needed
##########
src/java/org/apache/cassandra/db/SystemKeyspace.java:
##########
Review Comment:
Can you please check `SystemKeyspaceMigrator41.java`? specifically
`migrateCompactionHistory()` method. You might need to include compaction_type
during migration.
##########
test/unit/org/apache/cassandra/db/SystemKeyspaceTest.java:
##########
@@ -158,6 +158,34 @@ public void testPersistLocalMetadata()
assertEquals(DatabaseDescriptor.getStoragePort(),
row.getInt("listen_port"));
}
+ @Test
+ public void testCompactionHistory()
+ {
+ String ks = "test_ks";
+ String cf = "test_cf";
+ long now = System.currentTimeMillis();
+ Map<Integer, Long> rowsMerged = Collections.singletonMap(1, 100L);
+ Map<String, String> props = Collections.singletonMap("strategy",
"STCS");
+ String compactionType = "TestMajor";
+
+ SystemKeyspace.updateCompactionHistory(
+ org.apache.cassandra.utils.TimeUUID.Generator.nextTimeUUID(),
Review Comment:
```suggestion
TimeUUID.Generator.nextTimeUUID(),
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
