[GitHub] [hbase] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r436888210
##
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/master/snapshot/TestSnapshotFileCacheWithDifferentWorkingDir.java
##
@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master.snapshot;
+
+import java.io.File;
+import java.nio.file.Paths;
+import java.util.UUID;
+import org.apache.commons.io.FileUtils;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.snapshot.SnapshotDescriptionUtils;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.apache.hadoop.hbase.testclassification.MasterTests;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.experimental.categories.Category;
+
+/**
+ * Test that we correctly reload the cache, filter directories, etc.
+ * while the temporary directory is on a different file system than the root
directory
+ */
+@Category({MasterTests.class, LargeTests.class})
+public class TestSnapshotFileCacheWithDifferentWorkingDir extends
TestSnapshotFileCache {
Review comment:
I created HBASE-24522 to address this comment. Busy at this moment for
other priorities, will go back to this later.
Hope it is ok with you, @ndimiduk. I addressed your other two comments, and
going to push the patch after it passes the tests.
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] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r435632309
##
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/master/snapshot/TestSnapshotFileCacheWithDifferentWorkingDir.java
##
@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master.snapshot;
+
+import java.io.File;
+import java.nio.file.Paths;
+import java.util.UUID;
+import org.apache.commons.io.FileUtils;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.snapshot.SnapshotDescriptionUtils;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.apache.hadoop.hbase.testclassification.MasterTests;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.experimental.categories.Category;
+
+/**
+ * Test that we correctly reload the cache, filter directories, etc.
+ * while the temporary directory is on a different file system than the root
directory
+ */
+@Category({MasterTests.class, LargeTests.class})
+public class TestSnapshotFileCacheWithDifferentWorkingDir extends
TestSnapshotFileCache {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+
HBaseClassTestRule.forClass(TestSnapshotFileCacheWithDifferentWorkingDir.class);
+
+ protected static String TEMP_DIR =
+Paths.get("").toAbsolutePath().toString() + Path.SEPARATOR +
UUID.randomUUID().toString();
Review comment:
The purpose to use java.nio is get a different fileystem (local) than
hdfs in this case to test out the snapshot working_dir.
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] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r435630118
##
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/master/snapshot/TestSnapshotFileCache.java
##
@@ -85,32 +106,34 @@ public void cleanupFiles() throws Exception {
@Test
public void testLoadAndDelete() throws IOException {
-SnapshotFileCache cache = new SnapshotFileCache(fs, rootDir, PERIOD,
1000,
-"test-snapshot-file-cache-refresh", new SnapshotFiles());
+SnapshotFileCache cache = new SnapshotFileCache(fs, rootDir, workingFs,
workingDir, PERIOD,
+ 1000, "test-snapshot-file-cache-refresh", new SnapshotFiles());
createAndTestSnapshotV1(cache, "snapshot1a", false, true, false);
+createAndTestSnapshotV1(cache, "snapshot1b", true, true, false);
createAndTestSnapshotV2(cache, "snapshot2a", false, true, false);
+createAndTestSnapshotV2(cache, "snapshot2b", true, true, false);
}
@Test
public void testReloadModifiedDirectory() throws IOException {
-SnapshotFileCache cache = new SnapshotFileCache(fs, rootDir, PERIOD,
1000,
-"test-snapshot-file-cache-refresh", new SnapshotFiles());
+SnapshotFileCache cache = new SnapshotFileCache(fs, rootDir, workingFs,
workingDir, PERIOD,
+ 1000, "test-snapshot-file-cache-refresh", new SnapshotFiles());
-createAndTestSnapshotV1(cache, "snapshot1", false, true, false);
+createAndTestSnapshotV1(cache, "snapshot1v1", false, true, false);
// now delete the snapshot and add a file with a different name
-createAndTestSnapshotV1(cache, "snapshot1", false, false, false);
+createAndTestSnapshotV1(cache, "snapshot1v2", false, false, false);
Review comment:
reverted it back.
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] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r435608020
##
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/master/snapshot/TestSnapshotFileCacheWithDifferentWorkingDir.java
##
@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master.snapshot;
+
+import java.io.File;
+import java.nio.file.Paths;
+import java.util.UUID;
+import org.apache.commons.io.FileUtils;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.snapshot.SnapshotDescriptionUtils;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.apache.hadoop.hbase.testclassification.MasterTests;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.experimental.categories.Category;
+
+/**
+ * Test that we correctly reload the cache, filter directories, etc.
+ * while the temporary directory is on a different file system than the root
directory
+ */
+@Category({MasterTests.class, LargeTests.class})
+public class TestSnapshotFileCacheWithDifferentWorkingDir extends
TestSnapshotFileCache {
Review comment:
Let me try to change it to a parameterized test.
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] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r435607490
##
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/master/snapshot/TestSnapshotFileCacheWithDifferentWorkingDir.java
##
@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master.snapshot;
+
+import java.io.File;
+import java.nio.file.Paths;
+import java.util.UUID;
+import org.apache.commons.io.FileUtils;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.snapshot.SnapshotDescriptionUtils;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.apache.hadoop.hbase.testclassification.MasterTests;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.experimental.categories.Category;
+
+/**
+ * Test that we correctly reload the cache, filter directories, etc.
+ * while the temporary directory is on a different file system than the root
directory
+ */
+@Category({MasterTests.class, LargeTests.class})
+public class TestSnapshotFileCacheWithDifferentWorkingDir extends
TestSnapshotFileCache {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+
HBaseClassTestRule.forClass(TestSnapshotFileCacheWithDifferentWorkingDir.class);
+
+ protected static String TEMP_DIR =
+Paths.get("").toAbsolutePath().toString() + Path.SEPARATOR +
UUID.randomUUID().toString();
Review comment:
Hmm, I copied from an existing test case and did not look at it very
closely, take a close look.
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] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r435606974
##
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/master/snapshot/TestSnapshotFileCache.java
##
@@ -85,32 +106,34 @@ public void cleanupFiles() throws Exception {
@Test
public void testLoadAndDelete() throws IOException {
-SnapshotFileCache cache = new SnapshotFileCache(fs, rootDir, PERIOD,
1000,
-"test-snapshot-file-cache-refresh", new SnapshotFiles());
+SnapshotFileCache cache = new SnapshotFileCache(fs, rootDir, workingFs,
workingDir, PERIOD,
+ 1000, "test-snapshot-file-cache-refresh", new SnapshotFiles());
createAndTestSnapshotV1(cache, "snapshot1a", false, true, false);
+createAndTestSnapshotV1(cache, "snapshot1b", true, true, false);
createAndTestSnapshotV2(cache, "snapshot2a", false, true, false);
+createAndTestSnapshotV2(cache, "snapshot2b", true, true, false);
}
@Test
public void testReloadModifiedDirectory() throws IOException {
-SnapshotFileCache cache = new SnapshotFileCache(fs, rootDir, PERIOD,
1000,
-"test-snapshot-file-cache-refresh", new SnapshotFiles());
+SnapshotFileCache cache = new SnapshotFileCache(fs, rootDir, workingFs,
workingDir, PERIOD,
+ 1000, "test-snapshot-file-cache-refresh", new SnapshotFiles());
-createAndTestSnapshotV1(cache, "snapshot1", false, true, false);
+createAndTestSnapshotV1(cache, "snapshot1v1", false, true, false);
// now delete the snapshot and add a file with a different name
-createAndTestSnapshotV1(cache, "snapshot1", false, false, false);
+createAndTestSnapshotV1(cache, "snapshot1v2", false, false, false);
Review comment:
Oh, probably an over-engineering change I made, will read the comments
and revert.
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] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r434772239
##
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:
Yeah, let me change it back to fs, workingFs is misleading.
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] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r434769865
##
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:
completeSnapshot() is being used by some other test classes besides
TakeSnapshotHandler. What it does is doing a few checks and do a rename/copy (
a static utility method). SnapshotDescriptionUtils is a class which aggregates
set of utility methods, though the name is kind of misleading, it already has a
set of utility methods beyond SnapshotDescription. Putting a static utility
method in TakeSnapshotHandler does not seem a good fit to me, what do you think?
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] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r434744701
##
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, 1000,
+"test-snapshot-file-cache-refresh", new SnapshotFiles());
+try {
Review comment:
This try{} finally {} does not catch any exception, it is just used for
deleting directory created by the test. If getSnapshotsInProgress() throws out
any exception, it will fail the test.
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] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r434744521
##
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:
This try{} finally {} does not catch any exception, it is just used for
deleting directory created by the test. If getSnapshotsInProgress() throws out
any exception, it will fail the test.
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] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r434223497
##
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));
+
+if (!ArrayUtils.isEmpty(snapshotsInProgress)) {
+ for (FileStatus snapshot : snapshotsInProgress) {
+try {
+
snapshotInProgress.addAll(fileInspector.filesUnderSnapshot(snapshot.getPath()));
+} catch (CorruptedSnapshotException cse) {
+ LOG.debug("Corrupted in-progress snapshot file exception, ignored.",
cse);
Review comment:
yeah, it has multiple constructors, in this specific case, there is no
SnapshotDescription provided (reading SnapshotDescription results into
IOException due to corrupted/missing files). I changed log level from debug to
info
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] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r434218196
##
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:
A separate case for corrupted manifest files.
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] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r434218061
##
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:
It tests missing snapshot manifests will throw out
CorruptedSnapshotException and it is handled in getSnapshotsInProgress().
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] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r434213835
##
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:
Going through the context, it tests in case of corrupted snapshot
manifest file, it throws out CorruptedSnapshotException and it is handled in
getSnapshotsInProgress().
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] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
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,
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
+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
[GitHub] [hbase] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r434169674
##
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:
This is a very good catch, I updated the code and added a new set of
tests to cover snapshot_working_dir to be on a different filesystem than
rootdir.
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] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r432094150
##
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:
Thanks! Let me check it out.
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] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r431469964
##
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));
+
+if (!ArrayUtils.isEmpty(snapshotsInProgress)) {
+ for (FileStatus snapshot : snapshotsInProgress) {
+try {
+
snapshotInProgress.addAll(fileInspector.filesUnderSnapshot(snapshot.getPath()));
+} catch (CorruptedSnapshotException cse) {
+ LOG.debug("Corrupted in-progress snapshot file exception, ignored.",
cse);
Review comment:
It will be hard to put table name here. For an example, in the context
of ExportSnapshot job, at the destination cluster, the .snapshotinfo is written
to tmp directory first. There is no table existing yet, table may be cloned
from snapshot after ExportSnapshot job is done.
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] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r431464432
##
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));
+
+if (!ArrayUtils.isEmpty(snapshotsInProgress)) {
+ for (FileStatus snapshot : snapshotsInProgress) {
+try {
+
snapshotInProgress.addAll(fileInspector.filesUnderSnapshot(snapshot.getPath()));
+} catch (CorruptedSnapshotException cse) {
+ LOG.debug("Corrupted in-progress snapshot file exception, ignored.",
cse);
Review comment:
It is hard to say. The CorruptedSnapshotException will only be thrown
out when it reads a corrupted .snapshotinfo file. There are two cases, one is
normal, which .snapshotinfo is being written and it reads incomplete one; the
other is that it is a real corrupted .snapshotinfo. The original code assumes
the first case (maybe in reality, there are more first cases than the second
one). Change it to info level? The exception has the following info
```
throw new CorruptedSnapshotException("Couldn't read snapshot info from:" +
snapshotInfo, e);
```
It is the path of .snapshotinfo, it has snapshot name embedded in the path
(does not have table name). Let me see if table name can be added.
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
