Apache9 commented on code in PR #7552:
URL: https://github.com/apache/hbase/pull/7552#discussion_r3295705369
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStore.java:
##########
@@ -1775,6 +1776,204 @@ public void testReclaimChunkWhenScaning() throws
IOException {
}
}
+ @Test
+ public void testReaderRaceConditionDuringCompaction() throws Exception {
+ HBaseTestingUtil util = new HBaseTestingUtil();
+ util.startMiniCluster(1);
+
+ try {
+ Configuration conf = util.getConfiguration();
+
+ byte[] FAMILY = Bytes.toBytes("f");
+ byte[] Q = Bytes.toBytes("q");
+ TableName TABLE = TableName.valueOf("race_test");
+
+ TableDescriptor td = TableDescriptorBuilder.newBuilder(TABLE)
+ .setColumnFamily(
+ ColumnFamilyDescriptorBuilder.newBuilder(FAMILY)
+ .setMaxVersions(1)
+ .build())
+ .build();
+
+ util.getAdmin().createTable(td);
+
+ HRegion region =
+ util.getMiniHBaseCluster().getRegions(TABLE).get(0);
+
+ HStore store = region.getStore(FAMILY);
+
+ for (int i = 0; i < 4; i++) {
+ Put p = new Put(Bytes.toBytes("row-" + i));
+ p.addColumn(FAMILY, Q, Bytes.toBytes(i));
+ region.put(p);
+ region.flush(true); // force store file
+ }
+
+ // sanity check
+ assertEquals(4, store.getStorefilesCount());
+
+ DefaultStoreFileManager sfm =
+ (DefaultStoreFileManager) store.getStoreEngine().getStoreFileManager();
+
+ HStoreFile victim =
+ sfm.getStoreFiles().iterator().next();
+
+ AtomicReference<Throwable> failure = new AtomicReference<>();
+ CountDownLatch start = new CountDownLatch(1);
+ CountDownLatch done = new CountDownLatch(2);
+
+ Thread remover = new Thread(() -> {
+ try {
+ start.await();
+ victim.closeStoreFile(true); // async-style close
Review Comment:
So how could this happen in real production?
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStore.java:
##########
@@ -1775,6 +1776,204 @@ public void testReclaimChunkWhenScaning() throws
IOException {
}
}
+ @Test
+ public void testReaderRaceConditionDuringCompaction() throws Exception {
+ HBaseTestingUtil util = new HBaseTestingUtil();
Review Comment:
Better introduce a new test class if we need start a mini cluster here.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultStoreFileManager.java:
##########
@@ -179,9 +179,30 @@ public void addCompactionResults(Collection<HStoreFile>
newCompactedfiles,
// Let a background thread close the actual reader on these compacted
files and also
// ensure to evict the blocks from block cache so that they are no longer
in
// cache
- newCompactedfiles.forEach(HStoreFile::markCompactedAway);
- compactedfiles = ImmutableList.sortedCopyOf(storeFileComparator,
- Iterables.concat(compactedfiles, newCompactedfiles));
+ List<HStoreFile> filesToClose = new ArrayList<>(newCompactedfiles);
+ try {
+ HStoreFile.increaseStoreFilesRefeCount(newCompactedfiles);
+ newCompactedfiles.forEach(hStoreFile -> {
+ StoreFileReader reader = hStoreFile.getReader();
+ try {
+ if (reader == null) {
+ hStoreFile.initReader();
+ } else {
+ filesToClose.remove(hStoreFile);
+ }
+ } catch (IOException e) {
+ LOG.warn("Couldn't initialize reader for " + hStoreFile, e);
+ throw new RuntimeException(e);
Review Comment:
Just use a for loop and throw the IOException out directly?
And please add comments to describe the logic here? Why we need to call
initReader here?
--
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]
