[ https://issues.apache.org/jira/browse/HBASE-5547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13245987#comment-13245987 ]
jirapos...@reviews.apache.org commented on HBASE-5547: ------------------------------------------------------ ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4633/#review6672 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/HConstants.java <https://reviews.apache.org/r/4633/#comment14456> There is no mentioning of HFile in the name of znode. What if another archive scheme comes up in the future ? src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java <https://reviews.apache.org/r/4633/#comment14433> What is the counterpart for disableHFileBackup() where backup for all tables is enabled ? src/main/java/org/apache/hadoop/hbase/regionserver/HFileArchiveManager.java <https://reviews.apache.org/r/4633/#comment14435> License. src/main/java/org/apache/hadoop/hbase/regionserver/HFileArchiveManager.java <https://reviews.apache.org/r/4633/#comment14436> 'or not' is not needed. src/main/java/org/apache/hadoop/hbase/regionserver/HFileArchiveManager.java <https://reviews.apache.org/r/4633/#comment14437> Remove empty line. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java <https://reviews.apache.org/r/4633/#comment14438> white space. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java <https://reviews.apache.org/r/4633/#comment14441> Should we log if rss == null ? Since in that case we cannot perform desired archiving. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java <https://reviews.apache.org/r/4633/#comment14439> Should they be named hFileArchive{Manager,Tracker} ? src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java <https://reviews.apache.org/r/4633/#comment14440> The leading 'get ' is not necessary. src/main/java/org/apache/hadoop/hbase/util/HFileArchiveUtil.java <https://reviews.apache.org/r/4633/#comment14442> License. src/main/java/org/apache/hadoop/hbase/util/HFileArchiveUtil.java <https://reviews.apache.org/r/4633/#comment14443> Please check the return value from mkdirs(). src/main/java/org/apache/hadoop/hbase/util/HFileArchiveUtil.java <https://reviews.apache.org/r/4633/#comment14444> Please check the return value. Should resolveAndCopyFile() be enclosed in a try block ? src/main/java/org/apache/hadoop/hbase/util/HFileArchiveUtil.java <https://reviews.apache.org/r/4633/#comment14445> The code at line 92 doesn't 'keep' HFiles. src/main/java/org/apache/hadoop/hbase/util/HFileArchiveUtil.java <https://reviews.apache.org/r/4633/#comment14446> This doesn't match the fs.rename() call. src/main/java/org/apache/hadoop/hbase/util/HFileArchiveUtil.java <https://reviews.apache.org/r/4633/#comment14447> What about non-directory files ? src/main/java/org/apache/hadoop/hbase/util/HFileArchiveUtil.java <https://reviews.apache.org/r/4633/#comment14448> Should include file in log message. src/main/java/org/apache/hadoop/hbase/util/HFileArchiveUtil.java <https://reviews.apache.org/r/4633/#comment14449> Move this to line 197. src/main/java/org/apache/hadoop/hbase/util/HFileArchiveUtil.java <https://reviews.apache.org/r/4633/#comment14450> Should this be contingent on success being true ? src/main/java/org/apache/hadoop/hbase/util/HFileArchiveUtil.java <https://reviews.apache.org/r/4633/#comment14451> Move else if to line 219 and enclose LOG.debug in curly braces. src/main/java/org/apache/hadoop/hbase/zookeeper/HFileArchiveTracker.java <https://reviews.apache.org/r/4633/#comment14434> Add license, please. src/main/java/org/apache/hadoop/hbase/zookeeper/HFileArchiveTracker.java <https://reviews.apache.org/r/4633/#comment14452> Should this method return a boolean which indicates whether the tracker is enabled ? src/main/java/org/apache/hadoop/hbase/zookeeper/HFileArchiveTracker.java <https://reviews.apache.org/r/4633/#comment14453> variable table should be included in the log. src/main/java/org/apache/hadoop/hbase/zookeeper/HFileArchiveTracker.java <https://reviews.apache.org/r/4633/#comment14454> Remove first 'the'. src/main/java/org/apache/hadoop/hbase/zookeeper/HFileArchiveTracker.java <https://reviews.apache.org/r/4633/#comment14455> Why do we return when archiveHFileZNode exists ? - Ted On 2012-04-04 01:23:29, Jesse Yates wrote: bq. bq. ----------------------------------------------------------- bq. This is an automatically generated e-mail. To reply, visit: bq. https://reviews.apache.org/r/4633/ bq. ----------------------------------------------------------- bq. bq. (Updated 2012-04-04 01:23:29) bq. bq. bq. Review request for hbase, Michael Stack and Lars Hofhansl. bq. bq. bq. Summary bq. ------- bq. bq. Essentially, whenever an hfile would be deleted, it is instead moved to the archive directory. In this impl, the archive directory is on a per table basis, but defaults to '.archive'. Removing hfiles occurs in three places - compaction, merge and catalog janitor. The former and two latter are distinctly different code paths, but but did pull out some similarities. The latter two end up calling the same method, so there should be a reasonable amount of overlap. bq. bq. Implementation wise: bq. Updated the HMasterInterface to pass the calls onto the zookeeper. bq. Added a zk listener to handle updates from the master to the RS to backup. bq. Added a utility for removing files and finding archive directories bq. Added tests for the regionserver and catalogjanitor approaches. bq. Added creation of manager in regionserver. bq. bq. bq. This addresses bug HBASE-5547. bq. https://issues.apache.org/jira/browse/HBASE-5547 bq. bq. bq. Diffs bq. ----- bq. bq. src/main/java/org/apache/hadoop/hbase/HConstants.java 21ac4ba bq. src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 16e4017 bq. src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java ea7ae45 bq. src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd bq. src/main/java/org/apache/hadoop/hbase/master/HMaster.java 9bd4ace bq. src/main/java/org/apache/hadoop/hbase/regionserver/HFileArchiveManager.java PRE-CREATION bq. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1d11f8f bq. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 4f80999 bq. src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java 6884d53 bq. src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 0c7b396 bq. src/main/java/org/apache/hadoop/hbase/util/HFileArchiveUtil.java PRE-CREATION bq. src/main/java/org/apache/hadoop/hbase/zookeeper/HFileArchiveTracker.java PRE-CREATION bq. src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 0f83655 bq. src/main/resources/hbase-default.xml 341431a bq. src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java d2b3060 bq. src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java 227c5f2 bq. src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionHFileArchiving.java PRE-CREATION bq. src/test/java/org/apache/hadoop/hbase/util/MockRegionServerServices.java bb3ddd7 bq. bq. Diff: https://reviews.apache.org/r/4633/diff bq. bq. bq. Testing bq. ------- bq. bq. Added two tests for the separate cases - archiving via the regionserver and for the catalog tracker. Former runs in a mini cluster and also touches the changes to HMasterInterface and zookeeper. bq. bq. bq. Thanks, bq. bq. Jesse bq. bq. > Don't delete HFiles when in "backup mode" > ----------------------------------------- > > Key: HBASE-5547 > URL: https://issues.apache.org/jira/browse/HBASE-5547 > Project: HBase > Issue Type: New Feature > Reporter: Lars Hofhansl > Assignee: Jesse Yates > > This came up in a discussion I had with Stack. > It would be nice if HBase could be notified that a backup is in progress (via > a znode for example) and in that case either: > 1. rename HFiles to be delete to <file>.bck > 2. rename the HFiles into a special directory > 3. rename them to a general trash directory (which would not need to be tied > to backup mode). > That way it should be able to get a consistent backup based on HFiles (HDFS > snapshots or hard links would be better options here, but we do not have > those). > #1 makes cleanup a bit harder. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira