----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57876/#review170001 -----------------------------------------------------------
Fix it, then Ship it! Hi Szabi, Your change looks good, however, I have some minor findings on the top of Anna's comments, please find them below. I ran 'ant clean test' and the 3rd party tests with your patch, everything succeeded. Thanks, Bogi src/test/org/apache/sqoop/manager/oracle/OracleSpecialCharacterTableImportTest.java Lines 115 (patched) <https://reviews.apache.org/r/57876/#comment242729> Could you please use here a more self-explanatory naming? src/test/org/apache/sqoop/manager/oracle/OracleSpecialCharacterTableImportTest.java Lines 131 (patched) <https://reviews.apache.org/r/57876/#comment242740> I got a "new blank line at EOF" warning during applying your patch because of this line. - Boglarka Egyed On March 23, 2017, 2:21 p.m., Szabolcs Vasas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57876/ > ----------------------------------------------------------- > > (Updated March 23, 2017, 2:21 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-3159 > https://issues.apache.org/jira/browse/SQOOP-3159 > > > Repository: sqoop-trunk > > > Description > ------- > > The fix itself could be done by rewriting line > org/apache/sqoop/orm/CompilationManager.java:303 from > > int indexOfDollarSign = chompedPath.indexOf("$"); > > to > > int indexOfDollarSign = chompedPath.lastIndexOf("$"); > > because that block is supposed to get the outer class name from the class > name but a class name could contain a $ sign as well. > So for example the outer class name of class Foo$Bar$1 is be Foo$Bar. > > However to make the unit testing easier and make the purpose of that method > clearer I did some refactoring in > org.apache.sqoop.orm.CompilationManager#addClassFilesFromDir method. > > > Diffs > ----- > > src/java/org/apache/sqoop/orm/CompilationManager.java c1a656b > src/test/com/cloudera/sqoop/TestExport.java df5a663 > src/test/com/cloudera/sqoop/manager/OracleExportTest.java ec56cbe > > src/test/org/apache/sqoop/manager/oracle/OracleSpecialCharacterTableImportTest.java > PRE-CREATION > src/test/org/apache/sqoop/orm/TestCompilationManager.java PRE-CREATION > > > Diff: https://reviews.apache.org/r/57876/diff/2/ > > > Testing > ------- > > ant clean test > > New unit tests and third party tests are added and ran successfully. > > > Thanks, > > Szabolcs Vasas > >
