keith-turner commented on code in PR #3555:
URL: https://github.com/apache/accumulo/pull/3555#discussion_r1248022027
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java:
##########
@@ -1247,20 +1247,21 @@ private Optional<CompactionInfo>
reserveFilesForCompaction(CompactionServiceId s
// check is done after the file are exclusively reserved in this class to
avoid race conditions.
if (!tablet.getDatafiles().keySet().containsAll(cInfo.jobFiles)) {
// The tablet does not know of all these files, so unreserve them.
- completeCompaction(job, cInfo.jobFiles, Optional.empty(), true);
+ completeCompaction(job, cInfo.jobFiles, Optional.empty());
return Optional.empty();
}
return Optional.of(cInfo);
}
private void completeCompaction(CompactionJob job, Set<StoredTabletFile>
jobFiles,
- Optional<StoredTabletFile> metaFile, boolean successful) {
+ Optional<StoredTabletFile> metaFile) {
synchronized (this) {
Preconditions.checkState(removeJob(job));
- if (successful) {
- fileMgr.completed(job, jobFiles, metaFile);
- }
+ // If the compaction failed, then metaFile will be null.
+ // This is expected and the set of files for the compaction
+ // will be released so that it can be retried.
+ fileMgr.completed(job, jobFiles, metaFile);
Review Comment:
This successful flag was an attempt to handle a special case. The special
case is that compaction can be successful and produce an empty file. This
happens when a compaction produces no data, like its iterators filter out all
data. So can not assume an empty output file means failure.
Not completely sure, but I think to handle this special case we want to keep
the successful flag and pass it like `fileMgr.completed(job, jobFiles,
metaFile, successful)`. Then inside that method there is an if stmt, maybe add
a check for successful to the following if stmt.
```java
if (successful && (job.getKind() == CompactionKind.USER || job.getKind() ==
CompactionKind.SELECTOR)) {
selectedCompactionCompleted(job, jobFiles, newFile);
}
```
Could also add a precondition arg check that newFile is empty when
successful is false.
CompactionIT has a test testMultiStepCompactionThatDeletesAll that may test
this case, not completely sure. With user compactions a set of files is
selected and we only want to remove from the selected set when the compaction
is successful.
--
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]
