----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8559/#review28072 -----------------------------------------------------------
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java <https://reviews.apache.org/r/8559/#comment54619> Could these go in a central place? Either in a accumulo.Constants calss or AccumuloUtil? src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java <https://reviews.apache.org/r/8559/#comment54617> Just curious, is there a reason you went for throwing an error rather than using the equivalent of having EMPTY_BYTES for these, the way the convenience methods in Mutation (http://bit.ly/16VR93I) handle a missing visibility? src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java <https://reviews.apache.org/r/8559/#comment54618> When ACCUMULO_TEST_MODE is set, won't all of this error out? In the event that the job set up uses the test mode to create a MAC (and sets the ZK and instance names correctly), then the line getting the test mode option isn't needed. src/java/org/apache/sqoop/accumulo/MutationTransformer.java <https://reviews.apache.org/r/8559/#comment54620> Recommend Iterable<Mutation> instead of List, so that other implementations have an easier time not materializing the whole list in memory. src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java <https://reviews.apache.org/r/8559/#comment54621> Doesn't this fail in ACCUMULO_TEST_MODE? This looks like the place where we should set up the MAC, presuming we can overwrite the Accumulo connection information. src/java/org/apache/sqoop/tool/BaseSqoopTool.java <https://reviews.apache.org/r/8559/#comment54622> If I specified ACCUMULO_TEST_MODE, I don't need to provide these options, yes? src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java <https://reviews.apache.org/r/8559/#comment54624> Could you add a test class that ensures that when we use ACCUMULO_TEST_MODE, nothing is written to the accumulo instance specified in the connection arguments? src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java <https://reviews.apache.org/r/8559/#comment54623> It looks like Sqoop uses JUnit 4.11 to run tests, which means they could be run in parallel (though I don't know if Sqoop is usually built this way). At a minimum, this needs a strong warning about not being thread safe. it should probably be rewritten with a lock. and maybe an AtomicInteger, which you could use to do reference counting so that stopping can happen in the tearDown method. ( http://bit.ly/182jsOe and http://bit.ly/1iCYeeE ) - Sean Busbey On Oct. 29, 2013, 9:22 p.m., Philip Grim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8559/ > ----------------------------------------------------------- > > (Updated Oct. 29, 2013, 9:22 p.m.) > > > Review request for accumulo, Sqoop and Jarek Cecho. > > > Repository: sqoop-trunk > > > Description > ------- > > Adds the ability to import to an Accumulo table in much the same manner as > the current HBase import capability. Reported in JIRA as ACCUMULO-141 and > SQOOP-767. > > > Diffs > ----- > > ivy.xml c5130ae > src/docs/user/accumulo-args.txt PRE-CREATION > src/docs/user/accumulo.txt PRE-CREATION > src/java/org/apache/sqoop/SqoopOptions.java 13637b5 > src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java > PRE-CREATION > src/java/org/apache/sqoop/accumulo/AccumuloUtil.java PRE-CREATION > src/java/org/apache/sqoop/accumulo/MutationTransformer.java PRE-CREATION > src/java/org/apache/sqoop/accumulo/ToStringMutationTransformer.java > PRE-CREATION > src/java/org/apache/sqoop/manager/SqlManager.java 1ffa40f > src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java PRE-CREATION > src/java/org/apache/sqoop/mapreduce/AccumuloImportMapper.java PRE-CREATION > src/java/org/apache/sqoop/tool/BaseSqoopTool.java 9230f82 > src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d > src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java PRE-CREATION > src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java PRE-CREATION > src/test/org/apache/sqoop/accumulo/TestAccumuloQueryImport.java > PRE-CREATION > src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java PRE-CREATION > > Diff: https://reviews.apache.org/r/8559/diff/ > > > Testing > ------- > > > Thanks, > > Philip Grim > >
