You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2020/05/27 17:35:16 UTC
[GitHub] [hbase] huaxiangsun opened a new pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
huaxiangsun opened a new pull request #1791:
URL: https://github.com/apache/hbase/pull/1791
…t directory takes longer than cleaner TTL
----------------------------------------------------------------
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
[GitHub] [hbase] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
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<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:
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:
users@infra.apache.org
[GitHub] [hbase] huaxiangsun commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
huaxiangsun commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-635095629
Test failure is a flaky, HBASE-24429
----------------------------------------------------------------
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
[GitHub] [hbase] ndimiduk commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r431486691
##########
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));
+
+ 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:
`CorruptedSnapshotException` contains a copy of `SnapshotDescription`. Where this is logged, instead of logging the exception, it can grab the snaps tho info out of the description field to build a more meaningful log message.
I don't know about the log level. I guess it depends on if you think this is a really severe problem or not. If not, `debug` is fine. If it's the only sign an operator has of a bad problem, or it should be `info` or `warn`.
----------------------------------------------------------------
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
[GitHub] [hbase] Apache-HBase commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-634968890
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 6s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 47s | master passed |
| +1 :green_heart: | compile | 1m 9s | master passed |
| +1 :green_heart: | shadedjars | 6m 25s | branch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 48s | hbase-server in master failed. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 52s | the patch passed |
| +1 :green_heart: | compile | 1m 14s | the patch passed |
| +1 :green_heart: | javac | 1m 14s | the patch passed |
| +1 :green_heart: | shadedjars | 6m 26s | patch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 40s | hbase-server in the patch failed. |
||| _ Other Tests _ |
| -1 :x: | unit | 236m 34s | hbase-server in the patch failed. |
| | | 267m 11s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.9 Server=19.03.9 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1791 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 6f84e372f262 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 48e9835adb |
| Default Java | 2020-01-14 |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/1/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/1/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/1/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/1/testReport/ |
| Max. process+thread count | 3084 (vs. ulimit of 12500) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/1/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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
[GitHub] [hbase] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
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, 10000000,
+ "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:
users@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…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-634948652
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 4s | Docker mode activated. |
| -0 :warning: | yetus | 0m 2s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 3s | master passed |
| +1 :green_heart: | compile | 0m 58s | master passed |
| +1 :green_heart: | shadedjars | 6m 6s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 37s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 45s | the patch passed |
| +1 :green_heart: | compile | 0m 55s | the patch passed |
| +1 :green_heart: | javac | 0m 55s | the patch passed |
| +1 :green_heart: | shadedjars | 6m 2s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 36s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 197m 11s | hbase-server in the patch passed. |
| | | 223m 10s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.9 Server=19.03.9 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1791 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 1ab2864134df 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 48e9835adb |
| Default Java | 1.8.0_232 |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/1/testReport/ |
| Max. process+thread count | 3593 (vs. ulimit of 12500) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/1/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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
[GitHub] [hbase] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-637976835
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 33s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 27s | master passed |
| +1 :green_heart: | compile | 1m 6s | master passed |
| +1 :green_heart: | shadedjars | 5m 54s | branch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 45s | hbase-server in master failed. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 6s | the patch passed |
| +1 :green_heart: | compile | 1m 4s | the patch passed |
| +1 :green_heart: | javac | 1m 4s | the patch passed |
| +1 :green_heart: | shadedjars | 5m 53s | patch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 39s | hbase-server in the patch failed. |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 126m 1s | hbase-server in the patch passed. |
| | | 152m 37s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.11 Server=19.03.11 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/3/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1791 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 999780d57651 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / f71f1cdfa0 |
| Default Java | 2020-01-14 |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/3/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/3/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/3/testReport/ |
| Max. process+thread count | 4426 (vs. ulimit of 12500) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/3/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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
[GitHub] [hbase] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
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:
users@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…
Posted by GitBox <gi...@apache.org>.
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, 10000000,
+ "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:
users@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…
Posted by GitBox <gi...@apache.org>.
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, 10000000,
- "test-snapshot-file-cache-refresh", new SnapshotFiles());
+ SnapshotFileCache cache = new SnapshotFileCache(fs, rootDir, workingFs, workingDir, PERIOD,
+ 10000000, "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, 10000000,
- "test-snapshot-file-cache-refresh", new SnapshotFiles());
+ SnapshotFileCache cache = new SnapshotFileCache(fs, rootDir, workingFs, workingDir, PERIOD,
+ 10000000, "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:
users@infra.apache.org
[GitHub] [hbase] z-york commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
z-york commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-638516415
Looks good to me. Thanks for the changes. Let's wait to see if @ndimiduk has any further comments.
----------------------------------------------------------------
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
[GitHub] [hbase] huaxiangsun commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
huaxiangsun commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-634827703
This is a rebase of old PR, minor changes, no code logic change.
https://github.com/apache/hbase/pull/769
@busbey @z-york Please take a look, thanks.
----------------------------------------------------------------
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
[GitHub] [hbase] ndimiduk commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r435577323
##########
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, 10000000,
- "test-snapshot-file-cache-refresh", new SnapshotFiles());
+ SnapshotFileCache cache = new SnapshotFileCache(fs, rootDir, workingFs, workingDir, PERIOD,
+ 10000000, "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, 10000000,
- "test-snapshot-file-cache-refresh", new SnapshotFiles());
+ SnapshotFileCache cache = new SnapshotFileCache(fs, rootDir, workingFs, workingDir, PERIOD,
+ 10000000, "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:
I missed the reason for changing this test parameter. By the comment there between method invocations, it seems repetition of the name, a.k.a, the directory, "snapshot1" was intentional. Why have two different parameters? Doesn't that negate the purpose of this test?
##########
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:
nit: you're mixing `java.nio` and Hadoop APIs for building this path. I'm not quite sure, but maybe you want something like `Paths.get(".", UUID.randomUUID().toString()).toAbsolutePath().toString()`? I'm not sure where `Paths.get("")` would drop you, but I assume it's the same as `pwd`.
##########
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:
nit: it would require a good bit of refactor, but I find that test case inheritance is really unreliable to maintain. Maybe better to make `TestSnapshotFileCache` 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:
users@infra.apache.org
[GitHub] [hbase] ndimiduk commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r431403678
##########
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));
+
+ 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:
Could this be handled better? It seems that in the event of an exception, the entirety of the in-progress snapshot would excluded.
Logging the exception doesn't tell us anything useful because the `SnapshotDescription` isn't party to the message. Maybe we should log the snapshot name and table name. At WARN level?
----------------------------------------------------------------
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
[GitHub] [hbase] z-york commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-638501372
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 31s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 5m 18s | master passed |
| +1 :green_heart: | compile | 1m 19s | master passed |
| +1 :green_heart: | shadedjars | 6m 56s | branch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 47s | hbase-server in master failed. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 5m 11s | the patch passed |
| +1 :green_heart: | compile | 1m 19s | the patch passed |
| +1 :green_heart: | javac | 1m 19s | the patch passed |
| +1 :green_heart: | shadedjars | 7m 0s | patch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 47s | hbase-server in the patch failed. |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 207m 13s | hbase-server in the patch passed. |
| | | 239m 24s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.11 Server=19.03.11 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/4/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1791 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux afee4a7af76b 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 0078268203 |
| Default Java | 2020-01-14 |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/4/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/4/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/4/testReport/ |
| Max. process+thread count | 3037 (vs. ulimit of 12500) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/4/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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
[GitHub] [hbase] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-637911694
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 24s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 44s | master passed |
| +1 :green_heart: | compile | 0m 53s | master passed |
| +1 :green_heart: | shadedjars | 5m 36s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 38s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 25s | the patch passed |
| +1 :green_heart: | compile | 0m 56s | the patch passed |
| +1 :green_heart: | javac | 0m 56s | the patch passed |
| +1 :green_heart: | shadedjars | 5m 31s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 35s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 134m 14s | hbase-server in the patch passed. |
| | | 159m 7s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.11 Server=19.03.11 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/2/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1791 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux d85d69081ba7 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / cb5c0d2782 |
| Default Java | 1.8.0_232 |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/2/testReport/ |
| Max. process+thread count | 4376 (vs. ulimit of 12500) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/2/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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
[GitHub] [hbase] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
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<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));
+
+ 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:
users@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…
Posted by GitBox <gi...@apache.org>.
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<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));
+
+ 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:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-637979510
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 31s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 41s | master passed |
| +1 :green_heart: | compile | 0m 56s | master passed |
| +1 :green_heart: | shadedjars | 5m 35s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 39s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 23s | the patch passed |
| +1 :green_heart: | compile | 0m 53s | the patch passed |
| +1 :green_heart: | javac | 0m 53s | the patch passed |
| +1 :green_heart: | shadedjars | 5m 29s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 34s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 135m 28s | hbase-server in the patch failed. |
| | | 159m 22s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.11 Server=19.03.11 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/3/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1791 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 2f03ebdea3bc 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / f71f1cdfa0 |
| Default Java | 1.8.0_232 |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/3/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/3/testReport/ |
| Max. process+thread count | 4669 (vs. ulimit of 12500) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/3/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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
[GitHub] [hbase] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
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, 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:
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:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-638474023
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 36s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 48s | master passed |
| +1 :green_heart: | compile | 0m 57s | master passed |
| +1 :green_heart: | shadedjars | 5m 36s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 40s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 29s | the patch passed |
| +1 :green_heart: | compile | 0m 54s | the patch passed |
| +1 :green_heart: | javac | 0m 54s | the patch passed |
| +1 :green_heart: | shadedjars | 5m 31s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 37s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 139m 28s | hbase-server in the patch passed. |
| | | 163m 36s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.11 Server=19.03.11 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/4/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1791 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 5120ea93e01a 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 0078268203 |
| Default Java | 1.8.0_232 |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/4/testReport/ |
| Max. process+thread count | 3771 (vs. ulimit of 12500) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/4/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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
[GitHub] [hbase] z-york commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by 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
[GitHub] [hbase] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
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<String> filesUnderSnapshot(final Path snapshotDir)
+ public Collection<String> 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:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-638411813
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 36s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 47s | master passed |
| +1 :green_heart: | checkstyle | 1m 9s | master passed |
| +1 :green_heart: | spotbugs | 2m 1s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 25s | the patch passed |
| -0 :warning: | checkstyle | 1m 6s | hbase-server: The patch generated 5 new + 38 unchanged - 4 fixed = 43 total (was 42) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 11m 13s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1. |
| +1 :green_heart: | spotbugs | 2m 37s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 15s | The patch does not generate ASF License warnings. |
| | | 35m 27s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.11 Server=19.03.11 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/4/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1791 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
| uname | Linux ad3ca0482ebd 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 0078268203 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/4/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| Max. process+thread count | 94 (vs. ulimit of 12500) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/4/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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
[GitHub] [hbase] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-634851033
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 23s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 38s | master passed |
| +1 :green_heart: | checkstyle | 1m 32s | master passed |
| +1 :green_heart: | spotbugs | 2m 30s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 56s | the patch passed |
| +1 :green_heart: | checkstyle | 1m 9s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 12m 23s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1. |
| +1 :green_heart: | spotbugs | 2m 18s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 13s | The patch does not generate ASF License warnings. |
| | | 38m 11s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.9 Server=19.03.9 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1791 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
| uname | Linux ec69bbecbe95 4.15.0-74-generic #84-Ubuntu SMP Thu Dec 19 08:06:28 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 48e9835adb |
| Max. process+thread count | 84 (vs. ulimit of 12500) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/1/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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
[GitHub] [hbase] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
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, 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:
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:
users@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…
Posted by GitBox <gi...@apache.org>.
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<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));
+
+ 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:
users@infra.apache.org
[GitHub] [hbase] ndimiduk commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#discussion_r437717024
##########
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:
Yep, no problem. Thanks for addressing my other concerns :)
----------------------------------------------------------------
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
[GitHub] [hbase] z-york commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-640800579
----------------------------------------------------------------
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
[GitHub] [hbase] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
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, 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:
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:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-637932246
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 3m 32s | Docker mode activated. |
| -0 :warning: | yetus | 0m 4s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 5m 15s | master passed |
| +1 :green_heart: | compile | 1m 24s | master passed |
| +1 :green_heart: | shadedjars | 7m 6s | branch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 49s | hbase-server in master failed. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 5m 23s | the patch passed |
| +1 :green_heart: | compile | 1m 32s | the patch passed |
| +1 :green_heart: | javac | 1m 32s | the patch passed |
| +1 :green_heart: | shadedjars | 6m 54s | patch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 47s | hbase-server in the patch failed. |
||| _ Other Tests _ |
| -1 :x: | unit | 204m 24s | hbase-server in the patch failed. |
| | | 239m 3s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.11 Server=19.03.11 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/2/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1791 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 20b741f17d0e 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / cb5c0d2782 |
| Default Java | 2020-01-14 |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/2/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/2/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/2/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/2/testReport/ |
| Max. process+thread count | 2938 (vs. ulimit of 12500) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/2/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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
[GitHub] [hbase] Apache-HBase commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-637878987
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 34s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 47s | master passed |
| +1 :green_heart: | checkstyle | 1m 8s | master passed |
| +1 :green_heart: | spotbugs | 2m 3s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 23s | the patch passed |
| -0 :warning: | checkstyle | 1m 4s | hbase-server: The patch generated 5 new + 38 unchanged - 4 fixed = 43 total (was 42) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 11m 38s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1. |
| +1 :green_heart: | spotbugs | 2m 14s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 14s | The patch does not generate ASF License warnings. |
| | | 33m 59s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.11 Server=19.03.11 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1791 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
| uname | Linux ab2f654c03ff 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / cb5c0d2782 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/2/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| Max. process+thread count | 94 (vs. ulimit of 12500) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/2/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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
[GitHub] [hbase] huaxiangsun commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
huaxiangsun commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-637869217
@z-york @ndimiduk I updated the patch based on your comments. Here is a summary of the changes:
1. The major changes are addressing SNAPSHOT_WORKING_DIR config, added a new Test class to test against SNAPSHOT_WORKING_DIR to be a different filesystem from the rootdir's.
2. Other changes are for fixing testing issues.
Can you review the changes? Thanks.
p.s., The current state is that hfile cleaner and snapshot wont step on each other. The only missing part is ExportSnapshot. However, there is an issue in ExportSnapshot which does not honor the case that working dir/root dir are from different filesystem.
----------------------------------------------------------------
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
[GitHub] [hbase] huaxiangsun commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
huaxiangsun commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-638405644
@z-york I uploaded a new patch which addresses some of your comments, put up explain for rest of your comments. Please let me know if there is anything missing, thanks.
----------------------------------------------------------------
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
[GitHub] [hbase] huaxiangsun commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
huaxiangsun commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-638958405
Hi @ndimiduk, can you review the latest patch to see if there is anything missing? Thanks!
----------------------------------------------------------------
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
[GitHub] [hbase] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
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, 10000000,
+ "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:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1791:
URL: https://github.com/apache/hbase/pull/1791#issuecomment-637940900
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 34s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 48s | master passed |
| +1 :green_heart: | checkstyle | 1m 4s | master passed |
| +1 :green_heart: | spotbugs | 2m 4s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 21s | the patch passed |
| -0 :warning: | checkstyle | 1m 5s | hbase-server: The patch generated 5 new + 38 unchanged - 4 fixed = 43 total (was 42) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 11m 4s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1. |
| +1 :green_heart: | spotbugs | 3m 0s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 17s | The patch does not generate ASF License warnings. |
| | | 35m 3s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.11 Server=19.03.11 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/3/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1791 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
| uname | Linux bb13a89cc7bc 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / f71f1cdfa0 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/3/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| Max. process+thread count | 94 (vs. ulimit of 12500) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1791/3/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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
[GitHub] [hbase] huaxiangsun merged pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
huaxiangsun merged pull request #1791:
URL: https://github.com/apache/hbase/pull/1791
----------------------------------------------------------------
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
[GitHub] [hbase] huaxiangsun commented on a change in pull request #1791: HBASE-23202 ExportSnapshot (import) will fail if copying files to roo…
Posted by GitBox <gi...@apache.org>.
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<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:
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:
users@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…
Posted by GitBox <gi...@apache.org>.
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, 10000000,
- "test-snapshot-file-cache-refresh", new SnapshotFiles());
+ SnapshotFileCache cache = new SnapshotFileCache(fs, rootDir, workingFs, workingDir, PERIOD,
+ 10000000, "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, 10000000,
- "test-snapshot-file-cache-refresh", new SnapshotFiles());
+ SnapshotFileCache cache = new SnapshotFileCache(fs, rootDir, workingFs, workingDir, PERIOD,
+ 10000000, "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:
users@infra.apache.org