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<String> filesUnderSnapshot(final Path
snapshotDir)
+ public Collection<String> 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,
10000000,
+ "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 generate a data.manifest file
builder.consolidate();
builder.corruptDataManifest();
- fs.delete(SnapshotDescriptionUtils.getWorkingSnapshotDir(rootDir,
+ long period = Long.MAX_VALUE;
+ SnapshotFileCache cache = new SnapshotFileCache(conf, period, 10000000,
+ "test-snapshot-file-cache-refresh", new SnapshotFiles());
+ try {
Review comment:
Same 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.
For queries about this service, please contact Infrastructure at:
[email protected]
