huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r434210245



##########
File path: 
hbase-server/src/test/java/org/apache/hadoop/hbase/master/snapshot/TestSnapshotFileCache.java
##########
@@ -133,6 +145,71 @@ public void 
testCacheUpdatedWhenLastModifiedOfSnapDirNotUpdated() throws IOExcep
     createAndTestSnapshotV2(cache, "snapshot2v2", true, false, true);
   }
 
+  @Test
+  public void testWeNeverCacheTmpDirAndLoadIt() throws Exception {
+
+    final AtomicInteger count = new AtomicInteger(0);
+    // don't refresh the cache unless we tell it to
+    long period = Long.MAX_VALUE;
+    SnapshotFileCache cache = new SnapshotFileCache(fs, rootDir, period, 
10000000,
+        "test-snapshot-file-cache-refresh", new SnapshotFiles()) {
+      @Override
+      List<String> getSnapshotsInProgress()
+              throws IOException {
+        List<String> result = super.getSnapshotsInProgress();
+        count.incrementAndGet();
+        return result;
+      }
+
+      @Override public void triggerCacheRefreshForTesting() {
+        super.triggerCacheRefreshForTesting();
+      }
+    };
+
+    SnapshotMock.SnapshotBuilder complete =
+        createAndTestSnapshotV1(cache, "snapshot", false, false, false);
+
+    int countBeforeCheck = count.get();
+
+    CommonFSUtils.logFileSystemState(fs, rootDir, LOG);
+
+    List<FileStatus> allStoreFiles = getStoreFilesForSnapshot(complete);
+    Iterable<FileStatus> deletableFiles = 
cache.getUnreferencedFiles(allStoreFiles, null);
+    assertTrue(Iterables.isEmpty(deletableFiles));
+    // no need for tmp dir check as all files are accounted for.
+    assertEquals(0, count.get() - countBeforeCheck);
+
+    // add a random file to make sure we refresh
+    FileStatus randomFile = mockStoreFile(UTIL.getRandomUUID().toString());
+    allStoreFiles.add(randomFile);
+    deletableFiles = cache.getUnreferencedFiles(allStoreFiles, null);
+    assertEquals(randomFile, Iterables.getOnlyElement(deletableFiles));
+    assertEquals(1, count.get() - countBeforeCheck); // we check the tmp 
directory

Review comment:
       The newly added cases in testLoadAndDelete()
   +    createAndTestSnapshotV1(cache, "snapshot1b", true, true, false);
   +    createAndTestSnapshotV2(cache, "snapshot1a", true, true, false);
   test the case that files are correctly included for snapshot in progress.
   Is that not enough?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


Reply via email to