hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From st...@duboce.net
Subject Re: Review Request: 32-bit encoding of regionnames waaaaaaayyyyy too susceptible to hash clashes
Date Sat, 29 May 2010 21:50:26 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/104/#review100
-----------------------------------------------------------


It looks great to me.  Only objection is '/' as delimiter in new format.  Would prefer something
doesn't require escaping when searching in tools such as vim.  How about a '.'?  I'm going
to try it out now.  Will report back.


trunk/bin/add_table.rb
<http://review.hbase.org/r/104/#comment585>

    I'm sorry you had to edit this file Kannan



trunk/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java
<http://review.hbase.org/r/104/#comment586>

    Are you tied to '/'?  Could we use '.' instead?  I'm in vi and having to search for instances
of a regionname.  I'll have to escape the / before I begin my search?  If it was a '.' or
 '-' I wouldn't have to?



trunk/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java
<http://review.hbase.org/r/104/#comment587>

    This will come out funny in javadoc I think.  You might have to change the '<' into
< for them to show properly.



trunk/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java
<http://review.hbase.org/r/104/#comment588>

    Nice.  It'll be easy to change then.



trunk/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java
<http://review.hbase.org/r/104/#comment589>

    Nitpick: This could be written as:
    
    return (regionName.length >= 1) && (regionName[regionName.length - 1] == ENC_SEPARATOR);



trunk/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java
<http://review.hbase.org/r/104/#comment591>

    good



trunk/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java
<http://review.hbase.org/r/104/#comment592>

    Whats happening here?  Why not just return the cached String?  Does it not include the
new encoded suffix?



trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java
<http://review.hbase.org/r/104/#comment593>

    What changed on this line (and the one before).  Just watching out for you.  Any changes
in hfile cause Ryan to perk up and come looksee.



trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java
<http://review.hbase.org/r/104/#comment594>

    Be careful.  Looks only one real change in this file (though the diff reports many more
than that)



trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
<http://review.hbase.org/r/104/#comment595>

    This is great - getting rid of having to tag on the encoding



trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
<http://review.hbase.org/r/104/#comment596>

    What changed on this line?



trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
<http://review.hbase.org/r/104/#comment597>

    What changed on these lines?



trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
<http://review.hbase.org/r/104/#comment598>

    Yeah, what changed here?



trunk/src/main/java/org/apache/hadoop/hbase/util/MD5Hash.java
<http://review.hbase.org/r/104/#comment599>

    Throw a RuntimeException I'd say.



trunk/src/main/resources/hbase-webapps/master/table.jsp
<http://review.hbase.org/r/104/#comment600>

    This is good


- stack


On 2010-05-28 18:50:19, Kannan Muthukkaruppan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/104/
> -----------------------------------------------------------
> 
> (Updated 2010-05-28 18:50:19)
> 
> 
> Review request for hbase.
> 
> 
> Summary
> -------
> 
> The new format for a region name contains its encodedName. The encoded name also serves
as the directory name for the region in the filesystem.
> 
> New region name format:
> 
>       <tablename>,<startkey>,<regionIdTimestamp>/<encodedName>/
> 
> where, <encodedName> is a hex version of the MD5 hash of <tablename>,<startkey>,<regionIdTimestamp>
>  
> The old region name format remains:
>      <tablename>,<startkey>,<regionIdTimestamp>
> 
> For region names in the old format, the encoded name is a 32-bit JenkinsHash integer
value (in its decimal notation, string form). 
> 
> **NOTE**
>   
> ROOT, the first META region, and regions created by an older version of HBase (0.20 or
prior) will continue to use the old region name format.
> 
> 
> In the logs & web ui, old format region names will show up as:
>    <tablename>,<startkey>,<regionIdTimestamp>(<jenkinshashEncodedName>)
> New format region names will show up as:
>     <tablename>,<startkey>,<regionIdTimestamp>/<md5hashEncodedName>/
> 
> 
> This addresses bug HBASE-2531.
> 
> 
> Diffs
> -----
> 
>   trunk/bin/add_table.rb 949322 
>   trunk/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java 949322 
>   trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 949322 
>   trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 949322 
>   trunk/src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java 949322 
>   trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java 949322 
>   trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 949322 
>   trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 949322 
>   trunk/src/main/java/org/apache/hadoop/hbase/util/MD5Hash.java PRE-CREATION 
>   trunk/src/main/resources/hbase-webapps/master/table.jsp 949322 
>   trunk/src/main/resources/hbase-webapps/regionserver/regionserver.jsp 949322 
>   trunk/src/test/java/org/apache/hadoop/hbase/TestEmptyMetaInfo.java 949322 
>   trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompaction.java 949322

>   trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestGetClosestAtOrBefore.java
949322 
>   trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionInfo.java 949322

>   trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java 949322

> 
> Diff: http://review.hbase.org/r/104/diff
> 
> 
> Testing
> -------
> 
> unit tests pass. ran some cluster tests, and things seemed to work ok. Yet to try some
migration test (upgrading from an older server).
> 
> 
> Thanks,
> 
> Kannan
> 
>


Mime
View raw message