CASSANDRA-21065: Fix ConcurrentModificationException in performGarbageCollection with only_purge_repaired_tombstones #4571
Conversation
… by copying transaction.originals()
michaelsembwever
left a comment
michaelsembwever
left a comment
There was a problem hiding this comment.
lgtm, just nits for readability (and redundant docker approach)
test/unit/org/apache/cassandra/db/compaction/GarbageCollectRepairedSSTablesTest.java
Outdated
Show resolved
Hide resolved
test/unit/org/apache/cassandra/db/compaction/GarbageCollectRepairedSSTablesTest.java
Outdated
Show resolved
Hide resolved
test/unit/org/apache/cassandra/db/compaction/GarbageCollectRepairedSSTablesTest.java
Outdated
Show resolved
Hide resolved
test/unit/org/apache/cassandra/db/compaction/GarbageCollectRepairedSSTablesTest.java
Outdated
Show resolved
Hide resolved
test/unit/org/apache/cassandra/db/compaction/GarbageCollectRepairedSSTablesTest.java
Outdated
Show resolved
Hide resolved
test/unit/org/apache/cassandra/db/compaction/GarbageCollectRepairedSSTablesTest.java
Outdated
Show resolved
Hide resolved
test/unit/org/apache/cassandra/db/compaction/GarbageCollectRepairedSSTablesTest.java
Outdated
Show resolved
Hide resolved
src/java/org/apache/cassandra/db/compaction/CompactionManager.java
Outdated
Show resolved
Hide resolved
Co-authored-by: mck <mck@apache.org>
Remove redundant comments in the garbage collection test.
@michaelsembwever thank you for the review! All requested changes have been implemented. |
There was a problem hiding this comment.
above, and against the test methods there's repeated mention of the ConcurrentModificationException.
but that's not the only value of these test methods. and four of these methods don't even break without the patch, so have nothing to do with the ConcurrentModificationException bug– the test methods still add value, but their value-add can be documented more accurately.
There was a problem hiding this comment.
i think the first sentence above is more than enough.
but if you want to keep the second part and stacktrace snippet, at least do
| * Before CASSANDRA-21065 this would fail with: |
There was a problem hiding this comment.
what is the method actually testing then ?
Better to be direct about what the test method's value actually is. (Rather than describe what it is not.)
There was a problem hiding this comment.
There was a problem hiding this comment.
replace System.err with LOGGER.error
(all 11 occurrences in this file)
There was a problem hiding this comment.
curious) do you know how to reproduce this ?
There was a problem hiding this comment.
There was a problem hiding this comment.
2 participants
Fix ConcurrentModificationException in performGarbageCollection with only_purge_repaired_tombstones
When running
nodetool garbagecollecton a table with mixed repaired/unrepaired SSTables andonly_purge_repaired_tombstones=true, a ConcurrentModificationException was thrown because the code iterated directly overtransaction.originals()while callingtransaction.cancel()inside the loop, modifying the underlying collection.The fix copies the collection before iterating:
new ArrayList<>(transaction.originals())Tests run via: