> On 2012-03-14 03:05:47, Francis Liu wrote: > > http://svn.apache.org/repos/asf/incubator/hcatalog/trunk/src/java/org/apache/hcatalog/pig/HCatStorer.java, > > line 80 > > <https://reviews.apache.org/r/4322/diff/1/?file=91790#file91790line80> > > > > nitpick: use getProperty isntead so you don't have to cast it to string.
Done. > On 2012-03-14 03:05:47, Francis Liu wrote: > > http://svn.apache.org/repos/asf/incubator/hcatalog/trunk/storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/HBaseHCatStorageHandler.java, > > line 89 > > <https://reviews.apache.org/r/4322/diff/2/?file=91963#file91963line89> > > > > nitpick: since you're changing this can you fix the formatting? :-) sure. > On 2012-03-14 03:05:47, Francis Liu wrote: > > http://svn.apache.org/repos/asf/incubator/hcatalog/trunk/storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/HBaseHCatStorageHandler.java, > > line 112 > > <https://reviews.apache.org/r/4322/diff/2/?file=91963#file91963line112> > > > > I'd rather add it directly to JobConf. That way it's less of a hack and > > less dependent on an unplanned behavior. Done. > On 2012-03-14 03:05:47, Francis Liu wrote: > > http://svn.apache.org/repos/asf/incubator/hcatalog/trunk/storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/HBaseHCatStorageHandler.java, > > line 128 > > <https://reviews.apache.org/r/4322/diff/2/?file=91963#file91963line128> > > > > add a TODO to remove this once HCAT-308 is fixed. Done. > On 2012-03-14 03:05:47, Francis Liu wrote: > > http://svn.apache.org/repos/asf/incubator/hcatalog/trunk/storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/HBaseHCatStorageHandler.java, > > line 195 > > <https://reviews.apache.org/r/4322/diff/2/?file=91963#file91963line195> > > > > shouldn't you add it to copy? for the output you don't need the hack of > > changing conf do you? Yes. Will remove it when we remove for the input after HCAT-308. Just put in there so it is similar to input. > On 2012-03-14 03:05:47, Francis Liu wrote: > > http://svn.apache.org/repos/asf/incubator/hcatalog/trunk/storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/HBaseHCatStorageHandler.java, > > line 163 > > <https://reviews.apache.org/r/4322/diff/2/?file=91963#file91963line163> > > > > Does this work without the foolproof code? Let's try to contain the bug > > with pig in HCatStorer. It'll just add more complexity. Nothing complex. Just added to check to ensure we don't create transaction again if same jobConf is passed again with a different OutputJobInfo. In Pig's case we fixed to pass same OutputJobInfo but it is with a different JobConf . Added a test case assert for the two cases. - Rohini ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4322/#review5917 ----------------------------------------------------------- On 2012-03-14 01:07:44, Rohini Palaniswamy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4322/ > ----------------------------------------------------------- > > (Updated 2012-03-14 01:07:44) > > > Review request for hcatalog, Alan Gates, Francis Liu, and Vandana > Ayyalasomayajula. > > > Summary > ------- > > Fixes to get Hbase storagehandler working with Pig. > > > 1)Includes changes to HCatStorer used by Pig. Modified HCatStorer to pass the > same OutputJobInfo in the multiple calls made in Pig to > HCatOutputFormat.setOutput. Else we ended up creating multiple orphaned > transactions which were never committed in zookeeper. > 2) Moved stuff from getInputSplits and checkoutputSpecs to configure methods > in storagehandler. > 3) Fixed a logic error in hbase record reader. > > > This addresses bug HCATALOG-302. > https://issues.apache.org/jira/browse/HCATALOG-302 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/incubator/hcatalog/trunk/src/java/org/apache/hcatalog/pig/HCatStorer.java > 1299921 > > http://svn.apache.org/repos/asf/incubator/hcatalog/trunk/storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/HBaseBulkOutputFormat.java > 1299921 > > http://svn.apache.org/repos/asf/incubator/hcatalog/trunk/storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/HBaseDirectOutputFormat.java > 1299921 > > http://svn.apache.org/repos/asf/incubator/hcatalog/trunk/storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/HBaseHCatStorageHandler.java > 1299921 > > http://svn.apache.org/repos/asf/incubator/hcatalog/trunk/storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/HBaseInputFormat.java > 1299921 > > http://svn.apache.org/repos/asf/incubator/hcatalog/trunk/storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/HbaseSnapshotRecordReader.java > 1299921 > > http://svn.apache.org/repos/asf/incubator/hcatalog/trunk/storage-handlers/hbase/src/test/org/apache/hcatalog/hbase/TestHBaseBulkOutputFormat.java > 1299921 > > http://svn.apache.org/repos/asf/incubator/hcatalog/trunk/storage-handlers/hbase/src/test/org/apache/hcatalog/hbase/TestHBaseDirectOutputFormat.java > 1299921 > > http://svn.apache.org/repos/asf/incubator/hcatalog/trunk/storage-handlers/hbase/src/test/org/apache/hcatalog/hbase/TestHBaseInputFormat.java > 1299921 > > Diff: https://reviews.apache.org/r/4322/diff > > > Testing > ------- > > Unit tests and Integration testing done. > > > Thanks, > > Rohini > >
