[GitHub] [hbase] huaxiangsun commented on a change in pull request #769: HBASE-23202 ExportSnapshot (import) will fail if copying files to root directory takes longer than cleaner TTL

2020-05-27 Thread GitBox


huaxiangsun commented on a change in pull request #769:
URL: https://github.com/apache/hbase/pull/769#discussion_r431324046



##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotFileCache.java
##
@@ -251,6 +261,31 @@ private void refreshCache() throws IOException {
 this.snapshots.putAll(newSnapshots);
   }
 
+  @VisibleForTesting
+  List getSnapshotsInProgress() throws IOException {
+List snapshotInProgress = Lists.newArrayList();
+// only add those files to the cache, but not to the known snapshots
+Path snapshotTmpDir = new Path(snapshotDir, 
SnapshotDescriptionUtils.SNAPSHOT_TMP_DIR_NAME);
+FileStatus[] running = FSUtils.listStatus(fs, snapshotTmpDir);
+if (running != null) {
+  for (FileStatus run : running) {
+try {
+  
snapshotInProgress.addAll(fileInspector.filesUnderSnapshot(run.getPath()));
+} catch (CorruptedSnapshotException e) {
+  // See HBASE-16464
+  if (e.getCause() instanceof FileNotFoundException) {
+// If the snapshot is corrupt, we will delete it
+fs.delete(run.getPath(), true);
+LOG.warn("delete the " + run.getPath() + " due to exception:", 
e.getCause());

Review comment:
   I rebased the patch and posted a new pull request, 
   https://github.com/apache/hbase/pull/1791
   
   It is same as the original one, except some minor changes (like some of 
utilities are moved, change to use new utility class).





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [hbase] huaxiangsun commented on a change in pull request #769: HBASE-23202 ExportSnapshot (import) will fail if copying files to root directory takes longer than cleaner TTL

2020-05-26 Thread GitBox


huaxiangsun commented on a change in pull request #769:
URL: https://github.com/apache/hbase/pull/769#discussion_r430663274



##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotFileCache.java
##
@@ -251,6 +261,31 @@ private void refreshCache() throws IOException {
 this.snapshots.putAll(newSnapshots);
   }
 
+  @VisibleForTesting
+  List getSnapshotsInProgress() throws IOException {
+List snapshotInProgress = Lists.newArrayList();
+// only add those files to the cache, but not to the known snapshots
+Path snapshotTmpDir = new Path(snapshotDir, 
SnapshotDescriptionUtils.SNAPSHOT_TMP_DIR_NAME);
+FileStatus[] running = FSUtils.listStatus(fs, snapshotTmpDir);
+if (running != null) {
+  for (FileStatus run : running) {
+try {
+  
snapshotInProgress.addAll(fileInspector.filesUnderSnapshot(run.getPath()));
+} catch (CorruptedSnapshotException e) {
+  // See HBASE-16464
+  if (e.getCause() instanceof FileNotFoundException) {
+// If the snapshot is corrupt, we will delete it
+fs.delete(run.getPath(), true);
+LOG.warn("delete the " + run.getPath() + " due to exception:", 
e.getCause());

Review comment:
   Yeah, if it reads into the middle of copying manifest files, it is ok to 
remove this snapshot as copying HFiles has not started yet. So there is no 
impact for the logic in snapshotCleaner. 

##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotFileCache.java
##
@@ -251,6 +261,31 @@ private void refreshCache() throws IOException {
 this.snapshots.putAll(newSnapshots);
   }
 
+  @VisibleForTesting
+  List getSnapshotsInProgress() throws IOException {
+List snapshotInProgress = Lists.newArrayList();
+// only add those files to the cache, but not to the known snapshots
+Path snapshotTmpDir = new Path(snapshotDir, 
SnapshotDescriptionUtils.SNAPSHOT_TMP_DIR_NAME);
+FileStatus[] running = FSUtils.listStatus(fs, snapshotTmpDir);
+if (running != null) {
+  for (FileStatus run : running) {
+try {
+  
snapshotInProgress.addAll(fileInspector.filesUnderSnapshot(run.getPath()));
+} catch (CorruptedSnapshotException e) {
+  // See HBASE-16464
+  if (e.getCause() instanceof FileNotFoundException) {
+// If the snapshot is corrupt, we will delete it
+fs.delete(run.getPath(), true);
+LOG.warn("delete the " + run.getPath() + " due to exception:", 
e.getCause());

Review comment:
   The logic of getUnreferencedFiles() is that for an HFile which is not in 
cache, it will refreshCache to get the latest snapshot hfiles. If one hfile 
from this exortSnapshot job is in the list, this means that manifest files have 
been copied over, so refreshCache() will get the latest snapshot file list. 

##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotFileCache.java
##
@@ -251,6 +261,31 @@ private void refreshCache() throws IOException {
 this.snapshots.putAll(newSnapshots);
   }
 
+  @VisibleForTesting
+  List getSnapshotsInProgress() throws IOException {
+List snapshotInProgress = Lists.newArrayList();
+// only add those files to the cache, but not to the known snapshots
+Path snapshotTmpDir = new Path(snapshotDir, 
SnapshotDescriptionUtils.SNAPSHOT_TMP_DIR_NAME);
+FileStatus[] running = FSUtils.listStatus(fs, snapshotTmpDir);
+if (running != null) {
+  for (FileStatus run : running) {
+try {
+  
snapshotInProgress.addAll(fileInspector.filesUnderSnapshot(run.getPath()));
+} catch (CorruptedSnapshotException e) {
+  // See HBASE-16464
+  if (e.getCause() instanceof FileNotFoundException) {
+// If the snapshot is corrupt, we will delete it
+fs.delete(run.getPath(), true);
+LOG.warn("delete the " + run.getPath() + " due to exception:", 
e.getCause());

Review comment:
   @busbey @z-york Unless you see something missing, I think this one is 
good to go, 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:
us...@infra.apache.org