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