bdeggleston commented on code in PR #2976:
URL: https://github.com/apache/cassandra/pull/2976#discussion_r1420982064
##########
src/java/org/apache/cassandra/db/compaction/CompactionStrategyManager.java:
##########
@@ -749,8 +763,16 @@ else if (i < a.length)
*/
private void handleFlushNotification(Iterable<SSTableReader> added)
{
- for (SSTableReader sstable : added)
- compactionStrategyFor(sstable).addSSTable(sstable);
+ List<GroupedSSTableContainer> groups = groupSSTables(added);
Review Comment:
it seems like the intent here is to add all sstables with a single lock
acquisition from PendingRepairManager, but I'm having trouble seeing why that
matters and/or how not doing that can put compaction/repair in a bad state. It
might race with a call to `PendingRepairManager#removeSessionIfEmpty`, but the
next call to addSStable will add a strategy for the repair id
##########
src/java/org/apache/cassandra/db/compaction/PendingRepairManager.java:
##########
@@ -483,6 +483,14 @@ public Collection<AbstractCompactionTask>
createUserDefinedTasks(Collection<SSTa
return group.entrySet().stream().map(g ->
strategies.get(g.getKey()).getUserDefinedTask(g.getValue(),
gcBefore)).collect(Collectors.toList());
}
+ public boolean hasPendingRepairSSTable(TimeUUID sessionID, SSTableReader
sstable)
Review Comment:
could you add `@VisibleForTesting` for this and the intermediate method in
`PendingRepairHolder`? If it's not threadsafe, I'd also add `unsafe` to the
method names, along with an explanation of why it's unsafe
--
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]
