----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/87/#review96 -----------------------------------------------------------
src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java <http://review.hbase.org/r/87/#comment554> @Ryan: This shouldn't block commit of Todd's patch though, right? We can move it out after your refactoring? src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java <http://review.hbase.org/r/87/#comment555> Is this patch for trunk or branch? If branch, this addition might break RPC. It might be ok on the end here but we should test. src/main/java/org/apache/hadoop/hbase/mapreduce/Driver.java <http://review.hbase.org/r/87/#comment556> Is this a good description? Its underneath the Import.class which imports by going against the API. You might add a work about it going behind the API, writing hfiles? Hmmm... later I see that it can do both, write hfile or write to table src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat.java <http://review.hbase.org/r/87/#comment557> Nice! src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat.java <http://review.hbase.org/r/87/#comment558> Man, we never enable asserts.... throw an exception! src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat.java <http://review.hbase.org/r/87/#comment559> Just asking, the comparator for sure is going to do the right thing here? src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat.java <http://review.hbase.org/r/87/#comment560> Won't javadoc mangle your nice formatting here? Use ul/li or wrap w/ <pre>? src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat.java <http://review.hbase.org/r/87/#comment561> Why two types? Is this legacy? KV has advantage of being able to carry a Delete src/main/java/org/apache/hadoop/hbase/mapreduce/ImportTsv.java <http://review.hbase.org/r/87/#comment562> In KV, there is a parseColumns function that might help here src/main/java/org/apache/hadoop/hbase/mapreduce/ImportTsv.java <http://review.hbase.org/r/87/#comment563> Is this safe? Is there escaping you should be handling here? src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java <http://review.hbase.org/r/87/#comment564> I suppose in rare circumstances, it could split in here and mess you up. You need a no-split flag per region out in zk or something? We'd need such a thing for snapshotting methinks. But, thats for a later... src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java <http://review.hbase.org/r/87/#comment566> THis is good src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java <http://review.hbase.org/r/87/#comment565> Yeah, there is probably other metadata you'd want to bring over beyond blocksize -- like whether its compressed? For later. src/main/java/org/apache/hadoop/hbase/mapreduce/PutSortReducer.java <http://review.hbase.org/r/87/#comment567> Class comment. Whats this thing do? src/main/java/org/apache/hadoop/hbase/mapreduce/hadoopbackport/InputSampler.java <http://review.hbase.org/r/87/#comment568> We need this even though you wrote out partition edges earlier when you read region boundaries? src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java <http://review.hbase.org/r/87/#comment569> This is old cruddy stuff you are asking about. It looks like I added splitsAndClosesLock to stop either happening during critical periods (HBASE-588) and the splits lock looks now like a half measure from way back in 2007 -- HBASE-217. Lets make an issue to clean it up. src/main/java/org/apache/hadoop/hbase/regionserver/Store.java <http://review.hbase.org/r/87/#comment570> Nice src/main/java/org/apache/hadoop/hbase/regionserver/Store.java <http://review.hbase.org/r/87/#comment571> There is FSUtils in hbase util src/main/java/org/apache/hadoop/hbase/regionserver/Store.java <http://review.hbase.org/r/87/#comment572> What you going to do here? Get the last key from the map of storefiles in current region and make your file be one less than it? Your timestamp is current though so edits in here could overshadow those in another storefile. Thats ok? src/main/java/org/apache/hadoop/hbase/util/Bytes.java <http://review.hbase.org/r/87/#comment573> You were going to remove this src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java <http://review.hbase.org/r/87/#comment574> Good src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java <http://review.hbase.org/r/87/#comment575> Ugh... ok. src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java <http://review.hbase.org/r/87/#comment576> Whats this about? src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java <http://review.hbase.org/r/87/#comment577> Its not removing it itself? Its registered to cleanup on exit. src/test/java/org/apache/hadoop/hbase/mapreduce/NMapInputFormat.java <http://review.hbase.org/r/87/#comment578> How? If nullwritables, don't I just get nulls? - stack On 2010-05-25 14:21:16, Todd Lipcon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.hbase.org/r/87/ > ----------------------------------------------------------- > > (Updated 2010-05-25 14:21:16) > > > Review request for hbase, stack and Jonathan Gray. > > > Summary > ------- > > Here's a first patch that implements bulk import into existing tables. This > applies on top of HBASE-2586 and HBASE-2588 - I've pushed the series of the > three to my github: http://github.com/toddlipcon/hbase/tree/hfof-review > > I have some TODOs left that I want to take care of before this gets > committed, but since it's a pretty large patch, I figured I'd get it out for > review ASAP. > > The stuff in the hadoopbackport package is essentially copypaste from Hadoop > trunk, so you can ignore that in the review. > > > This addresses bug HBASE-1923. > http://issues.apache.org/jira/browse/HBASE-1923 > > > Diffs > ----- > > pom.xml 0a009cf > src/main/java/org/apache/hadoop/hbase/HRegionInfo.java 29b0cd6 > src/main/java/org/apache/hadoop/hbase/io/ImmutableBytesWritable.java > 0a9ec4b > src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java cf4768f > src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 4cbe52a > src/main/java/org/apache/hadoop/hbase/mapreduce/Driver.java 3d40695 > src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat.java > 9c8e53e > src/main/java/org/apache/hadoop/hbase/mapreduce/ImportTsv.java PRE-CREATION > src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java > PRE-CREATION > src/main/java/org/apache/hadoop/hbase/mapreduce/PutSortReducer.java > PRE-CREATION > > src/main/java/org/apache/hadoop/hbase/mapreduce/SimpleTotalOrderPartitioner.java > af3d588 > > src/main/java/org/apache/hadoop/hbase/mapreduce/hadoopbackport/InputSampler.java > PRE-CREATION > > src/main/java/org/apache/hadoop/hbase/mapreduce/hadoopbackport/TotalOrderPartitioner.java > PRE-CREATION > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 287cd48 > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java > fb65ed1 > src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 7de766d > src/main/java/org/apache/hadoop/hbase/util/Bytes.java a53dafe > src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java ed8709f > src/test/java/org/apache/hadoop/hbase/mapreduce/NMapInputFormat.java > PRE-CREATION > src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java > d04ced2 > > src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFiles.java > PRE-CREATION > src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionInfo.java > fcb22fb > > Diff: http://review.hbase.org/r/87/diff > > > Testing > ------- > > Primary unit/functional testing, a bit of pseudo-distributed testing. Plan on > doing full system tests before commit as well. > > > Thanks, > > Todd > >
