keith-turner commented on code in PR #2811:
URL: https://github.com/apache/accumulo/pull/2811#discussion_r982269825
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java:
##########
@@ -396,80 +396,89 @@ Set<StoredTabletFile> getCandidates(Set<StoredTabletFile>
currFiles, CompactionK
if (isCompactionStratConfigured)
return Set.of();
- switch (selectStatus) {
- case NOT_ACTIVE:
- case CANCELED: {
- Set<StoredTabletFile> candidates = new HashSet<>(currFiles);
- candidates.removeAll(allCompactingFiles);
- return Collections.unmodifiableSet(candidates);
- }
- case NEW:
- case SELECTING:
- return Set.of();
- case SELECTED: {
- Set<StoredTabletFile> candidates = new HashSet<>(currFiles);
- candidates.removeAll(allCompactingFiles);
- if (getNanoTime() - selectedTimeNanos <
selectedExpirationDuration.toNanos()) {
- candidates.removeAll(selectedFiles);
- }
- return Collections.unmodifiableSet(candidates);
- }
- case RESERVED: {
- Set<StoredTabletFile> candidates = new HashSet<>(currFiles);
- candidates.removeAll(allCompactingFiles);
- candidates.removeAll(selectedFiles);
- return Collections.unmodifiableSet(candidates);
- }
- default:
- throw new AssertionError();
- }
+ return handleSystemCompaction(currFiles);
}
case SELECTOR:
// intentional fall through
case USER:
- switch (selectStatus) {
- case NOT_ACTIVE:
- case NEW:
- case SELECTING:
- case CANCELED:
- return Set.of();
- case SELECTED:
- case RESERVED: {
- if (selectKind == kind) {
- Set<StoredTabletFile> candidates = new
HashSet<>(selectedFiles);
- candidates.removeAll(allCompactingFiles);
- candidates = Collections.unmodifiableSet(candidates);
- Preconditions.checkState(currFiles.containsAll(candidates),
- "selected files not in all files %s %s", candidates,
currFiles);
- return candidates;
- } else {
- return Set.of();
- }
- }
- default:
- throw new AssertionError();
- }
+ return handleUserSelectorCompaction(currFiles, kind);
case CHOP: {
- switch (chopStatus) {
- case NOT_ACTIVE:
- case SELECTING:
- case MARKING:
- return Set.of();
- case SELECTED: {
- if (selectStatus == FileSelectionStatus.NEW
- || selectStatus == FileSelectionStatus.SELECTING)
- return Set.of();
-
- var filesToChop = getFilesToChop(currFiles);
- filesToChop.removeAll(allCompactingFiles);
- if (selectStatus == FileSelectionStatus.SELECTED
- || selectStatus == FileSelectionStatus.RESERVED)
- filesToChop.removeAll(selectedFiles);
- return Collections.unmodifiableSet(filesToChop);
- }
- default:
- throw new AssertionError();
+ return handleChopCompaction(currFiles);
+ }
+ default:
+ throw new AssertionError();
+ }
+ }
+
+ private Set<StoredTabletFile> handleChopCompaction(Set<StoredTabletFile>
currFiles) {
+ switch (chopStatus) {
+ case NOT_ACTIVE:
+ case SELECTING:
+ case MARKING:
+ return Set.of();
+ case SELECTED: {
+ if (selectStatus == FileSelectionStatus.NEW
+ || selectStatus == FileSelectionStatus.SELECTING)
+ return Set.of();
+
+ var filesToChop = getFilesToChop(currFiles);
+ filesToChop.removeAll(allCompactingFiles);
+ if (selectStatus == FileSelectionStatus.SELECTED
+ || selectStatus == FileSelectionStatus.RESERVED)
+ filesToChop.removeAll(selectedFiles);
+ return Collections.unmodifiableSet(filesToChop);
+ }
+ default:
+ throw new AssertionError();
+ }
+ }
+
+ private Set<StoredTabletFile>
handleUserSelectorCompaction(Set<StoredTabletFile> currFiles,
+ CompactionKind kind) {
+ switch (selectStatus) {
+ case NOT_ACTIVE:
+ case NEW:
+ case SELECTING:
+ case CANCELED:
+ return Set.of();
+ case SELECTED:
+ case RESERVED: {
+ if (selectKind == kind) {
+ Set<StoredTabletFile> candidates = Sets.difference(selectedFiles,
allCompactingFiles);
+ Preconditions.checkState(currFiles.containsAll(candidates),
+ "selected files not in all files %s %s", candidates,
currFiles);
+ return Collections.unmodifiableSet(candidates);
Review Comment:
This code is called in a sync block in which `selectedFiles` and
`allCompactingFiles` will not be changing. The functions `Sets.difference()`
and `Collections.unmodifiableSet` may keep references to the passed in sets but
will not copy. So this function may return a set that internally references
`selectedFiles` and `allCompactingFiles` which may be changing after the method
returns. Its therefore important to copy as the original code did.
The following change will copy. I have no idea if its less or more
efficient than the original code, but I think `Sets.difference()` plus
`Set.copyOf()` is more readable than the original code so I would go with it.
```suggestion
//must create a copy because the sets passed to Sets.difference
could change after this method returns
return Set.copyOf(candidates);
```
I looked around at the other changes and I think all of those copy, but it
would be good if you could double check for any other cases like this in the
changes in this file.
--
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]
