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