----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/467/#review840 -----------------------------------------------------------
This is looking really great Li. If you can address the comments below in a new version, lets get this patch committed. Unfortunately, the master rewrite is going to change a bunch of the stuff that this patch depends on. For example, BaseScanner is going away. But, thats not your issue. What about scaling? The only problematic issue I saw was copy of storefiles on restore. We should file an issue to do that via a MR job. src/main/java/org/apache/hadoop/hbase/master/SnapshotMonitor.java <http://review.cloudera.org/r/467/#comment2820> final src/main/java/org/apache/hadoop/hbase/master/SnapshotMonitor.java <http://review.cloudera.org/r/467/#comment2821> Why let this out? src/main/java/org/apache/hadoop/hbase/master/SnapshotMonitor.java <http://review.cloudera.org/r/467/#comment2822> Want to remove this or enable the assertion? One or the other I'd say rather than this. src/main/java/org/apache/hadoop/hbase/master/SnapshotOperation.java <http://review.cloudera.org/r/467/#comment2823> This class looks good. src/main/java/org/apache/hadoop/hbase/master/SnapshotTracker.java <http://review.cloudera.org/r/467/#comment2824> Its a pity this class is named so. We're about to bring in a new patch that redoes the zk stuff -- breaks it up into pieces each with a singular purpose; e.g. tracking root location or tracking meta region server -- and unfortunately the pattern is to name these purposed classes *Tracker. There'll be a clash of this kinda Tracker and the new zk Trackers. Not important, just saying in case you have another name in mind for this class. src/main/java/org/apache/hadoop/hbase/master/SnapshotTracker.java <http://review.cloudera.org/r/467/#comment2825> Can you make this a configuration? int maxRetries = Configuration.getInt("hbase.snapshot.retries", 3); or something? It doesn't have to go into hbase-default.xml src/main/java/org/apache/hadoop/hbase/master/SnapshotTracker.java <http://review.cloudera.org/r/467/#comment2826> Yeah, can the max retries be made into a data member rather than hardcoded everywhere in this class? src/main/java/org/apache/hadoop/hbase/master/TableSnapshot.java <http://review.cloudera.org/r/467/#comment2827> Looks like you need more explaination here? This is a special case right where the table is offline and we're asked to make a snapshot of it? src/main/java/org/apache/hadoop/hbase/master/TableSnapshot.java <http://review.cloudera.org/r/467/#comment2828> Its not a backup, its creating references, right? src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java <http://review.cloudera.org/r/467/#comment2829> This looks like an improvement, making it static so can be used more generally. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java <http://review.cloudera.org/r/467/#comment2830> And flushing is disabled at this point too, right? Compactions? (Good). src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java <http://review.cloudera.org/r/467/#comment2831> If snapshot fails, do we have to do cleanup? src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java <http://review.cloudera.org/r/467/#comment2832> Good src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java <http://review.cloudera.org/r/467/#comment2833> This looks good. src/main/java/org/apache/hadoop/hbase/regionserver/SnapshotThread.java <http://review.cloudera.org/r/467/#comment2834> Call this Snapshotter instead? src/main/java/org/apache/hadoop/hbase/regionserver/Store.java <http://review.cloudera.org/r/467/#comment2835> Do we have to do this down at the Store level? Coud we move it up to Region or up to the RegionServer itself? It already has an HTable instance. src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java <http://review.cloudera.org/r/467/#comment2836> Why you have to pass the reference? It wasn't needed previously? src/main/java/org/apache/hadoop/hbase/util/FSUtils.java <http://review.cloudera.org/r/467/#comment2837> Does this stuff belong in here in this general utility class? Should it be polluted with References? Should this stuff be over in io package where the Reference is or static methods on Reference? src/test/java/org/apache/hadoop/hbase/master/TestSnapshot.java <http://review.cloudera.org/r/467/#comment2838> What about a test of restore from snapshot? Is there one? I dont' see it? src/test/java/org/apache/hadoop/hbase/master/TestSnapshotFailure.java <http://review.cloudera.org/r/467/#comment2839> Good test src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionSnapshot.java <http://review.cloudera.org/r/467/#comment2840> There is getTestDir(final String subdirName) FYI src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionSnapshot.java <http://review.cloudera.org/r/467/#comment2841> Looks good. src/test/java/org/apache/hadoop/hbase/regionserver/TestZKSnapshotWatcher.java <http://review.cloudera.org/r/467/#comment2842> Great - stack On 2010-08-09 03:52:11, Chongxin Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.cloudera.org/r/467/ > ----------------------------------------------------------- > > (Updated 2010-08-09 03:52:11) > > > Review request for hbase. > > > Summary > ------- > > This patch includes the first three sub-tasks of HBASE-50: > 1. Start and monitor the creation of snapshot via ZooKeeper > 2. Create snapshot of an HBase table > 3. Some existing functions of HBase are modified to support snapshot > > Currently snapshots can be created as expected, but can not be restored or > deleted yet > > > This addresses bug HBASE-50. > http://issues.apache.org/jira/browse/HBASE-50 > > > Diffs > ----- > > src/main/java/org/apache/hadoop/hbase/HConstants.java c77ebf5 > src/main/java/org/apache/hadoop/hbase/HRegionInfo.java ee94690 > src/main/java/org/apache/hadoop/hbase/HSnapshotDescriptor.java PRE-CREATION > src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java 0d57270 > src/main/java/org/apache/hadoop/hbase/SnapshotExistsException.java > PRE-CREATION > src/main/java/org/apache/hadoop/hbase/TablePartialOpenException.java > PRE-CREATION > src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 8b01aa0 > src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java ed12e7a > src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 85fde3a > src/main/java/org/apache/hadoop/hbase/io/Reference.java 219203c > src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java b2de7e4 > src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPCProtocolVersion.java > d4bcbed > src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java bd48a4b > src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java > 1183584 > src/main/java/org/apache/hadoop/hbase/master/BaseScanner.java 69eab39 > src/main/java/org/apache/hadoop/hbase/master/DeleteSnapshot.java > PRE-CREATION > src/main/java/org/apache/hadoop/hbase/master/HMaster.java e4bd30d > src/main/java/org/apache/hadoop/hbase/master/LogsCleaner.java 9d1a8b8 > src/main/java/org/apache/hadoop/hbase/master/RestoreSnapshot.java > PRE-CREATION > src/main/java/org/apache/hadoop/hbase/master/SnapshotLogCleaner.java > PRE-CREATION > src/main/java/org/apache/hadoop/hbase/master/SnapshotMonitor.java > PRE-CREATION > src/main/java/org/apache/hadoop/hbase/master/SnapshotOperation.java > PRE-CREATION > src/main/java/org/apache/hadoop/hbase/master/SnapshotTracker.java > PRE-CREATION > src/main/java/org/apache/hadoop/hbase/master/TableDelete.java 1153e62 > src/main/java/org/apache/hadoop/hbase/master/TableSnapshot.java > PRE-CREATION > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6dc41a4 > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java > 6a54736 > src/main/java/org/apache/hadoop/hbase/regionserver/SnapshotThread.java > PRE-CREATION > src/main/java/org/apache/hadoop/hbase/regionserver/Store.java ae9e190 > src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java 757a50c > src/main/java/org/apache/hadoop/hbase/regionserver/ZKSnapshotWatcher.java > PRE-CREATION > src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 9593286 > > src/main/java/org/apache/hadoop/hbase/replication/master/ReplicationLogCleaner.java > 4d4b00a > src/main/java/org/apache/hadoop/hbase/util/FSUtils.java 5cf3481 > src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java > 3827fa5 > src/main/resources/hbase-default.xml b73f0ff > src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 4d09fe9 > src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c9b78b9 > src/test/java/org/apache/hadoop/hbase/master/TestSnapshot.java PRE-CREATION > src/test/java/org/apache/hadoop/hbase/master/TestSnapshotFailure.java > PRE-CREATION > src/test/java/org/apache/hadoop/hbase/regionserver/TestCompaction.java > 34b8044 > src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 98bd3e5 > src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionSnapshot.java > PRE-CREATION > src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java > 38ef520 > > src/test/java/org/apache/hadoop/hbase/regionserver/TestZKSnapshotWatcher.java > PRE-CREATION > > Diff: http://review.cloudera.org/r/467/diff > > > Testing > ------- > > Unit tests and integration tests with mini cluster passed. > > > Thanks, > > Chongxin > >
