----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44716/#review123195 -----------------------------------------------------------
Thanks for taking this patch up Attila! Generally I like the approach, just few comments: src/java/org/apache/sqoop/SqoopOptions.java (line 109) <https://reviews.apache.org/r/44716/#comment185393> Let's name it as "temporary.dirRoot" - the "sqoop" prefix is not important here and I really want to get rid of the "test" portion as this propertly is not meant for test at all :) src/java/org/apache/sqoop/tool/BaseSqoopTool.java (line 165) <https://reviews.apache.org/r/44716/#comment185394> Might I suggest to rename the argument to "temporary-rootdir"? It's still descriptive and yet shorter :) src/java/org/apache/sqoop/util/AppendUtils.java <https://reviews.apache.org/r/44716/#comment185395> For users who are using this property already, we should provide a "backward" compatible way. Perhaps the default value in SqoopOptions should be System.getProperty("sqoop.test.import.rootDir", "_sqoop"); rather then just "_sqoop"? Jarcec - Jarek Cecho On March 11, 2016, 7:21 p.m., Attila Szabo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44716/ > ----------------------------------------------------------- > > (Updated March 11, 2016, 7:21 p.m.) > > > Review request for Sqoop and Jarek Cecho. > > > Repository: sqoop-trunk > > > Description > ------- > > Submitted changes for SQOOP-2880 > > > Diffs > ----- > > src/java/org/apache/sqoop/SqoopOptions.java 17751c7 > src/java/org/apache/sqoop/tool/BaseSqoopTool.java 50dd67d > src/java/org/apache/sqoop/tool/ImportTool.java ad1d48b > src/java/org/apache/sqoop/util/AppendUtils.java 0102ee7 > src/test/com/cloudera/sqoop/TestSqoopOptions.java f4660e5 > src/test/com/cloudera/sqoop/orm/TestParseMethods.java f222dd7 > > Diff: https://reviews.apache.org/r/44716/diff/ > > > Testing > ------- > > > Thanks, > > Attila Szabo > >
