hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From st...@duboce.net
Subject Re: Review Request: HBASE-50: Snapshot of table
Date Wed, 11 Aug 2010 04:34:40 GMT

-----------------------------------------------------------
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
> 
>


Mime
View raw message