Skip to content

Commit 5aa0fd2

Browse files
committed
HBASE-26675 Data race on Compactor.writer (#4035)
Signed-off-by: Xin Sun <ddupgs@gmail.com>
1 parent d9a2063 commit 5aa0fd2

File tree

2 files changed

+16
-16
lines changed

2 files changed

+16
-16
lines changed

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/Compactor.java

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,10 @@ public abstract class Compactor<T extends CellSink> {
9595
private final boolean dropCacheMajor;
9696
private final boolean dropCacheMinor;
9797

98-
protected T writer = null;
98+
// In compaction process only a single thread will access and write to this field, and
99+
// getCompactionTargets is the only place we will access it other than the compaction thread, so
100+
// make it volatile.
101+
protected volatile T writer = null;
99102

100103
//TODO: depending on Store is not good but, realistically, all compactors currently do.
101104
Compactor(Configuration conf, HStore store) {
@@ -546,17 +549,16 @@ protected InternalScanner createScanner(HStore store, ScanInfo scanInfo,
546549
dropDeletesFromRow, dropDeletesToRow);
547550
}
548551

549-
public List<Path> getCompactionTargets(){
550-
if (writer == null){
552+
public List<Path> getCompactionTargets() {
553+
T writer = this.writer;
554+
if (writer == null) {
551555
return Collections.emptyList();
552556
}
553-
synchronized (writer){
554-
if (writer instanceof StoreFileWriter){
555-
return Arrays.asList(((StoreFileWriter)writer).getPath());
556-
}
557-
return ((AbstractMultiFileWriter)writer).writers().stream().map(sfw -> sfw.getPath()).collect(
558-
Collectors.toList());
557+
if (writer instanceof StoreFileWriter) {
558+
return Arrays.asList(((StoreFileWriter) writer).getPath());
559559
}
560+
return ((AbstractMultiFileWriter) writer).writers().stream().map(sfw -> sfw.getPath())
561+
.collect(Collectors.toList());
560562
}
561563

562564
/**

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/DefaultCompactor.java

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -74,24 +74,22 @@ protected List<Path> commitWriter(FileDetails fd,
7474
@Override
7575
protected void abortWriter() throws IOException {
7676
abortWriter(writer);
77+
// this step signals that the target file is no longer written and can be cleaned up
78+
writer = null;
7779
}
7880

79-
protected void abortWriter(StoreFileWriter writer) throws IOException {
81+
protected final void abortWriter(StoreFileWriter writer) throws IOException {
8082
Path leftoverFile = writer.getPath();
8183
try {
8284
writer.close();
8385
} catch (IOException e) {
8486
LOG.warn("Failed to close the writer after an unfinished compaction.", e);
85-
} finally {
86-
//this step signals that the target file is no longer writen and can be cleaned up
87-
writer = null;
8887
}
8988
try {
9089
store.getFileSystem().delete(leftoverFile, false);
9190
} catch (IOException e) {
92-
LOG.warn(
93-
"Failed to delete the leftover file " + leftoverFile + " after an unfinished compaction.",
94-
e);
91+
LOG.warn("Failed to delete the leftover file {} after an unfinished compaction.",
92+
leftoverFile, e);
9593
}
9694
}
9795
}

0 commit comments

Comments
 (0)