[GitHub] [hbase] z-york commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…

[GitBox <gi...@apache.org>]

z-york commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r432052546



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotFileCache.java
##########
@@ -251,6 +260,25 @@ private void refreshCache() throws IOException {
     this.snapshots.putAll(newSnapshots);
   }
 
+  @VisibleForTesting
+  List<String> getSnapshotsInProgress() throws IOException {
+    List<String> snapshotInProgress = Lists.newArrayList();
+    // only add those files to the cache, but not to the known snapshots
+    FileStatus[] snapshotsInProgress = CommonFSUtils.listStatus(fs,
+      new Path(snapshotDir, SnapshotDescriptionUtils.SNAPSHOT_TMP_DIR_NAME));

Review comment:
       Please use https://github.com/apache/hbase/blob/master/hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java#L198 to get the temporary directory. Currently, this will break https://issues.apache.org/jira/browse/HBASE-21098. 
   
   It's probably worth adding a similar test with the config value for working snapshot dir set to avoid any regressions.

##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/snapshot/TestSnapshotHFileCleaner.java
##########
@@ -156,7 +164,29 @@ public void testCorruptedDataManifest() throws IOException {
     builder.consolidate();
     builder.corruptDataManifest();
 
-    fs.delete(SnapshotDescriptionUtils.getWorkingSnapshotDir(rootDir,
+    long period = Long.MAX_VALUE;
+    SnapshotFileCache cache = new SnapshotFileCache(fs, rootDir, period, 10000000,
+        "test-snapshot-file-cache-refresh", new SnapshotFiles());
+    try {
+      cache.getSnapshotsInProgress();
+    } finally {
+      fs.delete(SnapshotDescriptionUtils.getWorkingSnapshotDir(rootDir,
           TEST_UTIL.getConfiguration()), true);
+    }
+  }
+
+  @Test
+  public void testMissedTmpSnapshot() throws IOException {
+    SnapshotTestingUtils.SnapshotMock snapshotMock =
+        new SnapshotTestingUtils.SnapshotMock(TEST_UTIL.getConfiguration(), fs, rootDir);
+    SnapshotTestingUtils.SnapshotMock.SnapshotBuilder builder = snapshotMock.createSnapshotV2(
+        SNAPSHOT_NAME_STR, TABLE_NAME_STR);
+    builder.addRegionV2();
+    builder.missOneRegionSnapshotFile();
+    long period = Long.MAX_VALUE;
+    SnapshotFileCache cache = new SnapshotFileCache(fs, rootDir, period, 10000000,
+        "test-snapshot-file-cache-refresh", new SnapshotFiles());
+    cache.getSnapshotsInProgress();
+    assertTrue(fs.exists(builder.getSnapshotsDir()));

Review comment:
       What is this testing? 

##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/snapshot/TestSnapshotHFileCleaner.java
##########
@@ -137,8 +138,15 @@ public void testCorruptedRegionManifest() throws IOException {
     builder.addRegionV2();
     builder.corruptOneRegionManifest();
 
-    fs.delete(SnapshotDescriptionUtils.getWorkingSnapshotDir(rootDir, TEST_UTIL.getConfiguration()),
-      true);
+    long period = Long.MAX_VALUE;
+    SnapshotFileCache cache = new SnapshotFileCache(fs, rootDir, period, 10000000,
+        "test-snapshot-file-cache-refresh", new SnapshotFiles());
+    try {
+      cache.getSnapshotsInProgress();

Review comment:
       Is this testing that we don't throw an exception when the snapshot is corrupted?

##########
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:
       This tests the case where getSnapshotsInProgress is and isn't called, but we should also directly test the getSnapshotsInProgress method to test the case where a file is correctly included if the file is in progress (and the negative case).




----------------------------------------------------------------
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:
users@infra.apache.org