----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/467/#review823 -----------------------------------------------------------
I reviewed the first page of diffs. Will do second page tomorrow (This is a lovely big patch Li -- so far, so good... keep up the good work). src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java <http://review.cloudera.org/r/467/#comment2750> This is fine for an hbase that is a fresh install but what about case where the data has been migrated from an older hbase version; it won't have this column family in .META. We should make a little migration script that adds it or on start of new version, check for it and if not present, create it. src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java <http://review.cloudera.org/r/467/#comment2751> ditto src/main/java/org/apache/hadoop/hbase/TablePartialOpenException.java <http://review.cloudera.org/r/467/#comment2752> Maybe exception should be called TablePartiallyOpenException (Not important. Minor). src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java <http://review.cloudera.org/r/467/#comment2753> Can the snapshot name be empty and then we'll make one up? src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java <http://review.cloudera.org/r/467/#comment2754> Sure, why not. Its stored in the filesystem? If so, for sure use same rule. src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java <http://review.cloudera.org/r/467/#comment2755> For restore of the snapshot, do you use loadtable.rb or Todd's new bulkloading scripts? src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java <http://review.cloudera.org/r/467/#comment2756> This looks like an improvement. src/main/java/org/apache/hadoop/hbase/io/Reference.java <http://review.cloudera.org/r/467/#comment2757> Whats this? A different kind of reference? src/main/java/org/apache/hadoop/hbase/io/Reference.java <http://review.cloudera.org/r/467/#comment2758> Do you need to specify the ordinals? Wont they be these numbers anyways? Or is it in case someone adds a new type of reference? src/main/java/org/apache/hadoop/hbase/io/Reference.java <http://review.cloudera.org/r/467/#comment2759> Why not just use the ordinal? http://download-llnw.oracle.com/javase/1.5.0/docs/api/java/lang/Enum.html#ordinal() And serialize the int? src/main/java/org/apache/hadoop/hbase/io/Reference.java <http://review.cloudera.org/r/467/#comment2760> Hmm... is this good? You are dropping some the region name when you toString. Do we have to? src/main/java/org/apache/hadoop/hbase/io/Reference.java <http://review.cloudera.org/r/467/#comment2765> This could be a problem when fellas migrate old data to use this new hbase. If there are References in the old data, then this deserialization will fail? I'm fine w/ you creating a new issue named something like "Migration from 0.20.x hbase to 0.90" and adding a note in there that we need to consider this little issue. Better though would be if the data was able to migrate itself at runtime; i.e. recognize a boolean on the stream and then deserialize the old style into the new, etc. src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPCProtocolVersion.java <http://review.cloudera.org/r/467/#comment2767> Good. src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java <http://review.cloudera.org/r/467/#comment2768> Is this comment right? src/main/java/org/apache/hadoop/hbase/master/BaseScanner.java <http://review.cloudera.org/r/467/#comment2770> Why Random negative number? Why not just leave it blank? src/main/java/org/apache/hadoop/hbase/master/BaseScanner.java <http://review.cloudera.org/r/467/#comment2775> It'd be cool breaking up these methods so they were static if possible so you could unit test them to ensure they do as advertised. src/main/java/org/apache/hadoop/hbase/master/DeleteSnapshot.java <http://review.cloudera.org/r/467/#comment2776> Check the result. It may not have worked. If it didn't deserves a WARN at least. src/main/java/org/apache/hadoop/hbase/master/HMaster.java <http://review.cloudera.org/r/467/#comment2777> You might want to check the returns from these methods. src/main/java/org/apache/hadoop/hbase/master/HMaster.java <http://review.cloudera.org/r/467/#comment2778> Don't bother warning I'd say.. just throw src/main/java/org/apache/hadoop/hbase/master/HMaster.java <http://review.cloudera.org/r/467/#comment2779> Yeah, just throw... that'll show in logs anyway (I believe) src/main/java/org/apache/hadoop/hbase/master/HMaster.java <http://review.cloudera.org/r/467/#comment2780> I'm with Ted on this one. Need to do something about it else it'll annoy till the end of time. src/main/java/org/apache/hadoop/hbase/master/RestoreSnapshot.java <http://review.cloudera.org/r/467/#comment2781> This looks like it should be a general utility method (Not important) src/main/java/org/apache/hadoop/hbase/master/RestoreSnapshot.java <http://review.cloudera.org/r/467/#comment2782> If table were big, this could be prohibitively expensive? A single-threaded copy of all of a tables data? We could compliment this with MR-base restore, something that did the copy using MR? src/main/java/org/apache/hadoop/hbase/master/SnapshotLogCleaner.java <http://review.cloudera.org/r/467/#comment2784> This looks like a class that you could write a unit test for? - 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 > >
