You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "arvindKandpal-ksolves (via GitHub)" <gi...@apache.org> on 2025/12/22 07:45:45 UTC
[PR] CASSANDRA-20081: Add compaction type and strategy properties to compaction history [cassandra]
arvindKandpal-ksolves opened a new pull request, #4540:
URL: https://github.com/apache/cassandra/pull/4540
Currently, `nodetool compactionhistory` does not provide visibility into the specific type of compaction (e.g., Major, Minor, Validation) or the compaction strategy used (e.g., STCS, LCS). Additionally, the output displays raw bytes by default, which is difficult to read.
This patch enhances the compaction history by persisting the compaction type and strategy properties into the `system.compaction_history` table and exposing them via `nodetool`.
**Changes included in this patch:**
1. **Schema Update:** Added `compaction_type` column to `system.compaction_history`.
2. **Logic Update:** Updated `CompactionTask` to capture the compaction type (e.g., Major, Cleanup) and strategy class (e.g., SizeTieredCompactionStrategy) and persist them.
3. **JMX/Tooling:** Updated `CompactionHistoryTabularData` and `nodetool compactionhistory` printer to display these new fields.
4. **UX Improvement:** Changed `nodetool compactionhistory` to display human-readable sizes (KiB, MiB) by default. Added a `-n` / `--no-human-readable` flag to revert to raw bytes.
5. **Tests:** Updated `SystemKeyspaceTest`, `CompactionTaskTest`, and `CompactionHistoryTest` to cover schema changes and verify the persistence of new fields.
**Tests Performed:**
* `ant test -Dtest.name=SystemKeyspaceTest` (PASSED)
* `ant test -Dtest.name=CompactionTaskTest` (PASSED)
* `ant test -Dtest.name=CompactionHistoryTest` (PASSED)
* Manual verification using `nodetool compactionhistory` on a local node.
patch by arvindKandpal-ksolves ; for CASSANDRA-20081
--
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
Re: [PR] CASSANDRA-20081: Add compaction type and strategy properties to compaction history [cassandra]
Posted by "arvindKandpal-ksolves (via GitHub)" <gi...@apache.org>.
arvindKandpal-ksolves commented on PR #4540:
URL: https://github.com/apache/cassandra/pull/4540#issuecomment-3722554593
@jyothsnakonisa Thanks for the review. I have addressed all your comments:
1. Reverted nodetool flag changes to maintain backward compatibility.
2. Updated SystemKeyspaceMigrator41 and fixed upgrade tests.
3. Fixed code style, imports, and cleanup issues.
4. Ready for re-review.
--
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
Re: [PR] CASSANDRA-20081: Add compaction type and strategy properties to compaction history [cassandra]
Posted by "arvindKandpal-ksolves (via GitHub)" <gi...@apache.org>.
arvindKandpal-ksolves commented on PR #4540:
URL: https://github.com/apache/cassandra/pull/4540#issuecomment-3803036480
Committed as 6d60102422d8409cdf8fabfb1c906afdf6a85859
--
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
Re: [PR] CASSANDRA-20081: Add compaction type and strategy properties to compaction history [cassandra]
Posted by "jyothsnakonisa (via GitHub)" <gi...@apache.org>.
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: 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
Re: [PR] CASSANDRA-20081: Add compaction type and strategy properties to compaction history [cassandra]
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #4540:
URL: https://github.com/apache/cassandra/pull/4540#discussion_r2720212438
##########
src/java/org/apache/cassandra/tools/nodetool/stats/CompactionHistoryHolder.java:
##########
@@ -90,6 +90,7 @@ private HashMap<String, Object> getAllAsMap(boolean humanReadable)
compaction.put("bytes_out", FileUtils.stringifyFileSize(this.bytesOut, humanReadable));
compaction.put("rows_merged", this.rowMerged);
compaction.put("compaction_properties", this.compactionProperties);
+
Review Comment:
nit: delete newly added empty line
##########
test/unit/org/apache/cassandra/tools/nodetool/CompactionHistoryTest.java:
##########
@@ -102,35 +102,59 @@ public void testCompactionProperties() throws Throwable
ImmutableList.Builder<String> builder = ImmutableList.builder();
List<String> cmds = builder.addAll(cmd).add(keyspace()).add(currentTable()).build();
- compactionHistoryResultVerify(keyspace(), currentTable(), ImmutableMap.of(COMPACTION_TYPE_PROPERTY, compactionType), cmds);
- String cql = "select keyspace_name,columnfamily_name,compaction_properties from system." + SystemKeyspace.COMPACTION_HISTORY +
+ Map<String, String> baseProperties = ImmutableMap.of("strategy", "SizeTieredCompactionStrategy");
+ Map<String, String> expectedProperties = new HashMap<>(baseProperties);
+ expectedProperties.put("compaction_type", compactionType);
+
+ compactionHistoryResultVerify(keyspace(), currentTable(), expectedProperties, compactionType, cmds);
+
+ String cql = "select keyspace_name,columnfamily_name,compaction_properties from system." + SystemKeyspace.COMPACTION_HISTORY +
" where keyspace_name = '" + keyspace() + "' AND columnfamily_name = '" + currentTable() + "' ALLOW FILTERING";
+
Review Comment:
delete newly added empty line.
##########
src/java/org/apache/cassandra/db/compaction/CompactionHistoryTabularData.java:
##########
@@ -86,6 +87,7 @@ public static TabularData from(UntypedResultSet resultSet) throws OpenDataExcept
long bytesOut = row.getLong(ITEM_NAMES[5]);
Map<Integer, Long> rowMerged = row.getMap(ITEM_NAMES[6], Int32Type.instance, LongType.instance);
Map<String, String> compactionProperties = row.getMap(ITEM_NAMES[7], UTF8Type.instance, UTF8Type.instance);
+
Review Comment:
nit: delete new added empty line.
--
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
Re: [PR] CASSANDRA-20081: Add compaction type and strategy properties to compaction history [cassandra]
Posted by "arvindKandpal-ksolves (via GitHub)" <gi...@apache.org>.
arvindKandpal-ksolves commented on PR #4540:
URL: https://github.com/apache/cassandra/pull/4540#issuecomment-3733980202
Thanks @jyothsnakonisa. I will check with @smiklosovic regarding the CI run and request @Guoma for a second review on JIRA.
--
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
Re: [PR] CASSANDRA-20081: Add compaction type and strategy properties to compaction history [cassandra]
Posted by "jyothsnakonisa (via GitHub)" <gi...@apache.org>.
jyothsnakonisa commented on PR #4540:
URL: https://github.com/apache/cassandra/pull/4540#issuecomment-3730493513
Hi @arvindKandpal-ksolves @smiklosovic might be able to help you with that. I generally run it on my organization's infrastructure.
--
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
Re: [PR] CASSANDRA-20081: Add compaction type and strategy properties to compaction history [cassandra]
Posted by "arvindKandpal-ksolves (via GitHub)" <gi...@apache.org>.
arvindKandpal-ksolves closed pull request #4540: CASSANDRA-20081: Add compaction type and strategy properties to compaction history
URL: https://github.com/apache/cassandra/pull/4540
--
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
Re: [PR] CASSANDRA-20081: Add compaction type and strategy properties to compaction history [cassandra]
Posted by "arvindKandpal-ksolves (via GitHub)" <gi...@apache.org>.
arvindKandpal-ksolves commented on PR #4540:
URL: https://github.com/apache/cassandra/pull/4540#issuecomment-3728481272
Hi @jyothsnakonisa , thanks for the approval!
Could you please tell me how I can trigger the CI and what need to update in JIRA , as I am contributing to the code segment in Cassandra first time ?
--
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
Re: [PR] CASSANDRA-20081: Add compaction type and strategy properties to compaction history [cassandra]
Posted by "arvindKandpal-ksolves (via GitHub)" <gi...@apache.org>.
arvindKandpal-ksolves commented on PR #4540:
URL: https://github.com/apache/cassandra/pull/4540#issuecomment-3788854166
@Maxwell-Guo , I have updated the PR based on your feedback.
Reverted schema changes: Removed the new column.
Logic: compaction_type is now stored inside the compaction_properties map.
--
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
Re: [PR] CASSANDRA-20081: Add compaction type and strategy properties to compaction history [cassandra]
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #4540:
URL: https://github.com/apache/cassandra/pull/4540#discussion_r2703014417
##########
src/java/org/apache/cassandra/db/SystemKeyspace.java:
##########
@@ -373,6 +373,7 @@ private SystemKeyspace()
+ "keyspace_name text,"
+ "rows_merged map<int, bigint>,"
+ "compaction_properties frozen<map<text, text>>,"
+ + "compaction_type text,"
Review Comment:
Hi , why you add this column for table compaction_history ?
the column compaction_properties will also show the table's compaction type too.
we can see this jira https://issues.apache.org/jira/browse/CASSANDRA-18061
The first time i add the column compaction_properties is used to show the type of compaction task
and the column type is text too, but we changed it to a frozen map after some discussion.
--
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