blambov commented on code in PR #3532:
URL: https://github.com/apache/cassandra/pull/3532#discussion_r1761170255
##########
test/unit/org/apache/cassandra/db/compaction/CompactionStrategyManagerPendingRepairTest.java:
##########
@@ -306,6 +311,96 @@ public void cleanupCompactionFailed()
Assert.assertEquals(ActiveRepairService.UNREPAIRED_SSTABLE,
sstable.getSSTableMetadata().repairedAt);
}
+ /**
+ * Tests that finalized repairs racing with compactions on the same set of
sstables don't leave unrepaired sstables behind
+ *
+ * This test checks that when a repair has been finalized but there are
still pending sstables a finalize repair
+ * compaction task is issued for that repair session.
+ */
+ @Test
+ public void testFinalizedAndCompactionRace() throws IOException
+ {
+ UUID repairID = registerSession(cfs, true, true);
+ LocalSessionAccessor.prepareUnsafe(repairID, COORDINATOR,
PARTICIPANTS);
+
+ int numberOfSStables = 4;
+ List<SSTableReader> sstables = new ArrayList<>(numberOfSStables);
+ for (int i = 0; i < numberOfSStables; i++)
+ {
+ SSTableReader sstable = makeSSTable(true);
+ sstables.add(sstable);
+ Assert.assertFalse(sstable.isRepaired());
+ Assert.assertFalse(sstable.isPendingRepair());
+ }
+
+ // change to pending repair
+ mutateRepaired(sstables, repairID, false);
+ csm.handleNotification(new SSTableAddedNotification(sstables, null),
cfs.getTracker());
+ for (SSTableReader sstable : sstables)
+ {
+ Assert.assertFalse(sstable.isRepaired());
+ Assert.assertTrue(sstable.isPendingRepair());
+ Assert.assertEquals(repairID, sstable.getPendingRepair());
+ }
+
+ // Get a compaction taks based on the sstables marked as pending repair
+ cfs.getCompactionStrategyManager().enable();
+ for (SSTableReader sstable : sstables)
+ pendingContains(sstable);
+ AbstractCompactionTask compactionTask =
csm.getNextBackgroundTask(FBUtilities.nowInSeconds());
+ Assert.assertNotNull(compactionTask);
+
+ // Finalize the repair session
+ LocalSessionAccessor.finalizeUnsafe(repairID);
+ LocalSession session = ARS.consistent.local.getSession(repairID);
+ ARS.consistent.local.sessionCompleted(session);
+ Assert.assertTrue(hasPendingStrategiesFor(repairID));
+
+ // run the compaction
+ compactionTask.execute(ActiveCompactionsTracker.NOOP);
+
+ // The repair session is finalized but there is an sstable left behind
pending repair!
+ SSTableReader compactedSSTable =
cfs.getLiveSSTables().iterator().next();
+ Assert.assertEquals(repairID, compactedSSTable.getPendingRepair());
Review Comment:
Nit: Ideally the test wouldn't be checking that the transient invalid state
exists, only that everything is in correct state after the necessary actions
are taken.
I.e. it should also be acceptable for `compactedSSTable` to already be
marked as repaired, and in that case `getNextBackgroundTasks` below to return
nothing. I would thus wrap lines 364 to 381 inside an `if
(!compactedSSTable.isRepaired())`. This may also fix potential flakiness if the
thread is preempted before line 363 and something manages to run compaction
during the pause.
--
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]
