[GitHub] [hbase] z-york commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
z-york commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r434912178
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java
##
@@ -383,25 +385,38 @@ public static SnapshotDescription
readSnapshotInfo(FileSystem fs, Path snapshotD
}
/**
- * Move the finished snapshot to its final, publicly visible directory -
this marks the snapshot
- * as 'complete'.
- * @param snapshot description of the snapshot being tabken
- * @param rootdir root directory of the hbase installation
- * @param workingDir directory where the in progress snapshot was built
- * @param fs {@link FileSystem} where the snapshot was built
- * @throws org.apache.hadoop.hbase.snapshot.SnapshotCreationException if the
- * snapshot could not be moved
+ * Commits the snapshot process by moving the working snapshot
+ * to the finalized filepath
+ *
+ * @param snapshotDir The file path of the completed snapshots
+ * @param workingDir The file path of the in progress snapshots
+ * @param fs The file system of the completed snapshots
+ * @param workingDirFs The file system of the in progress snapshots
+ * @param conf Configuration
+ *
+ * @throws SnapshotCreationException if the snapshot could not be moved
* @throws IOException the filesystem could not be reached
*/
- public static void completeSnapshot(SnapshotDescription snapshot, Path
rootdir, Path workingDir,
- FileSystem fs) throws SnapshotCreationException, IOException {
-Path finishedDir = getCompletedSnapshotDir(snapshot, rootdir);
-LOG.debug("Snapshot is done, just moving the snapshot from " + workingDir
+ " to "
-+ finishedDir);
-if (!fs.rename(workingDir, finishedDir)) {
- throw new SnapshotCreationException(
- "Failed to move working directory(" + workingDir + ") to completed
directory("
- + finishedDir + ").", ProtobufUtil.createSnapshotDesc(snapshot));
+ public static void completeSnapshot(Path snapshotDir, Path workingDir,
FileSystem fs,
Review comment:
I think this is okay for now... maybe in the future we can pull these
non-snapshotDescription utility methods to a new file.
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
[GitHub] [hbase] z-york commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
z-york commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r434256730
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotHFileCleaner.java
##
@@ -93,12 +94,17 @@ public void setConf(final Configuration conf) {
DEFAULT_HFILE_CACHE_REFRESH_PERIOD);
final FileSystem fs = CommonFSUtils.getCurrentFileSystem(conf);
Path rootDir = CommonFSUtils.getRootDir(conf);
- cache = new SnapshotFileCache(fs, rootDir, cacheRefreshPeriod,
cacheRefreshPeriod,
- "snapshot-hfile-cleaner-cache-refresher", new
SnapshotFileCache.SnapshotFileInspector() {
+ Path workingDir =
SnapshotDescriptionUtils.getWorkingSnapshotDir(rootDir, conf);
+ FileSystem workingFs = workingDir.getFileSystem(conf);
+
+ cache = new SnapshotFileCache(fs, rootDir, workingFs, workingDir,
cacheRefreshPeriod,
+cacheRefreshPeriod, "snapshot-hfile-cleaner-cache-refresher",
+new SnapshotFileCache.SnapshotFileInspector() {
@Override
-public Collection filesUnderSnapshot(final Path
snapshotDir)
+public Collection filesUnderSnapshot(final FileSystem
workingFs,
Review comment:
This should just be be fs because workingFs or fs could be passed in
##
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,
1000,
+"test-snapshot-file-cache-refresh", new SnapshotFiles());
+try {
+ cache.getSnapshotsInProgress();
Review comment:
In this case, shouldn't the try{} be removed (since it might mask a true
failure)?
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java
##
@@ -383,25 +385,38 @@ public static SnapshotDescription
readSnapshotInfo(FileSystem fs, Path snapshotD
}
/**
- * Move the finished snapshot to its final, publicly visible directory -
this marks the snapshot
- * as 'complete'.
- * @param snapshot description of the snapshot being tabken
- * @param rootdir root directory of the hbase installation
- * @param workingDir directory where the in progress snapshot was built
- * @param fs {@link FileSystem} where the snapshot was built
- * @throws org.apache.hadoop.hbase.snapshot.SnapshotCreationException if the
- * snapshot could not be moved
+ * Commits the snapshot process by moving the working snapshot
+ * to the finalized filepath
+ *
+ * @param snapshotDir The file path of the completed snapshots
+ * @param workingDir The file path of the in progress snapshots
+ * @param fs The file system of the completed snapshots
+ * @param workingDirFs The file system of the in progress snapshots
+ * @param conf Configuration
+ *
+ * @throws SnapshotCreationException if the snapshot could not be moved
* @throws IOException the filesystem could not be reached
*/
- public static void completeSnapshot(SnapshotDescription snapshot, Path
rootdir, Path workingDir,
- FileSystem fs) throws SnapshotCreationException, IOException {
-Path finishedDir = getCompletedSnapshotDir(snapshot, rootdir);
-LOG.debug("Snapshot is done, just moving the snapshot from " + workingDir
+ " to "
-+ finishedDir);
-if (!fs.rename(workingDir, finishedDir)) {
- throw new SnapshotCreationException(
- "Failed to move working directory(" + workingDir + ") to completed
directory("
- + finishedDir + ").", ProtobufUtil.createSnapshotDesc(snapshot));
+ public static void completeSnapshot(Path snapshotDir, Path workingDir,
FileSystem fs,
Review comment:
Hmm, weird that this is duplicated. +1 on merging/removing one. Does it
make more sense to be part of TakeSnapshotHandler? Why does it need to be in
SnapshotDescriptionUtils?
##
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/master/snapshot/TestSnapshotHFileCleaner.java
##
@@ -148,15 +156,37 @@ public void testCorruptedRegionManifest() throws
IOException {
@Test
public void testCorruptedDataManifest() throws IOException {
SnapshotTestingUtils.SnapshotMock
-snapshotMock = new
SnapshotTestingUtils.SnapshotMock(TEST_UTIL.getConfiguration(), fs, rootDir);
+snapshotMock = new SnapshotTestingUtils.SnapshotMock(conf, fs,
rootDir);
SnapshotTestingUtils.SnapshotMock.SnapshotBuilder builder =
snapshotMock.createSnapshotV2(
SNAPSHOT_NAME_STR, TABLE_NAME_STR);
builder.addRegionV2();
// consolidate to gener
[GitHub] [hbase] z-york commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
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 getSnapshotsInProgress() throws IOException {
+List 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,
1000,
+"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,
1000,
+"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,
1000,
+"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,
1000,
+"test-snapshot-file-cache-refresh", new SnapshotFiles()) {
+ @Override
+ List getSnapshotsInProgress()
+ throws IOException {
+List 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 allStoreFiles = getStoreFilesForSnapshot(complete);
+Iterable 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
+
