----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2574/#review2908 -----------------------------------------------------------
Look good to me, Jarcec! Just some comments. /src/java/com/cloudera/sqoop/orm/ClassWriter.java <https://reviews.apache.org/r/2574/#comment6519> Similar to public static fields, also keep the signature in subclass and then call into superclass. /src/java/com/cloudera/sqoop/orm/ClassWriter.java <https://reviews.apache.org/r/2574/#comment6520> Same as above. /src/java/com/cloudera/sqoop/tool/SqoopTool.java <https://reviews.apache.org/r/2574/#comment6505> Remove trailing whitespace. - Bilung On 2011-10-28 06:51:43, Jarek Jarcec wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/2574/ > ----------------------------------------------------------- > > (Updated 2011-10-28 06:51:43) > > > Review request for Sqoop and Bilung Lee. > > > Summary > ------- > > Please check whether this is something that we're looking for. > > Just let me firstly explain the weirdness in inheritance between > com.cloudera.sqoop.tool.(Base)SqoopTool and > org.apache.sqoop.tool.(Base)SqoopTool (and other children). I initially > followed specification in master JIRA, but I've end up with not backward > compatible code that I have to fix on several places to get it compiled. > Problem was in broken inheritance topology - in old com.cloudera.sqoop > package there was main parent SqoopTool with child BaseSqoopTool and > additional children (ImportTool for example). With my original changes, > ImportTool was no longer child of com.cloudera.sqoop.tool.(Base)SqoopTool > because it was child of org.apache.sqoop.tool.(Base)SqoopTool instead. I > wasn't able to find better solution for this problem than having this weird > inheritance in place. > > If you have better idea how to get the job done, please don't hesitate to > advise. > > Jarcec > > > This addresses bug SQOOP-374. > https://issues.apache.org/jira/browse/SQOOP-374 > > > Diffs > ----- > > /src/java/com/cloudera/sqoop/orm/AvroSchemaGenerator.java 1190173 > /src/java/com/cloudera/sqoop/orm/ClassWriter.java 1190173 > /src/java/com/cloudera/sqoop/orm/CompilationManager.java 1190173 > /src/java/com/cloudera/sqoop/orm/TableClassName.java 1190173 > /src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java 1190173 > /src/java/com/cloudera/sqoop/tool/CodeGenTool.java 1190173 > /src/java/com/cloudera/sqoop/tool/CreateHiveTableTool.java 1190173 > /src/java/com/cloudera/sqoop/tool/EvalSqlTool.java 1190173 > /src/java/com/cloudera/sqoop/tool/ExportTool.java 1190173 > /src/java/com/cloudera/sqoop/tool/HelpTool.java 1190173 > /src/java/com/cloudera/sqoop/tool/ImportAllTablesTool.java 1190173 > /src/java/com/cloudera/sqoop/tool/ImportTool.java 1190173 > /src/java/com/cloudera/sqoop/tool/JobTool.java 1190173 > /src/java/com/cloudera/sqoop/tool/ListDatabasesTool.java 1190173 > /src/java/com/cloudera/sqoop/tool/ListTablesTool.java 1190173 > /src/java/com/cloudera/sqoop/tool/MergeTool.java 1190173 > /src/java/com/cloudera/sqoop/tool/MetastoreTool.java 1190173 > /src/java/com/cloudera/sqoop/tool/SqoopTool.java 1190173 > /src/java/com/cloudera/sqoop/tool/ToolDesc.java 1190173 > /src/java/com/cloudera/sqoop/tool/ToolPlugin.java 1190173 > /src/java/com/cloudera/sqoop/tool/VersionTool.java 1190173 > /src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java PRE-CREATION > /src/java/org/apache/sqoop/orm/ClassWriter.java PRE-CREATION > /src/java/org/apache/sqoop/orm/CompilationManager.java PRE-CREATION > /src/java/org/apache/sqoop/orm/TableClassName.java PRE-CREATION > /src/java/org/apache/sqoop/tool/BaseSqoopTool.java PRE-CREATION > /src/java/org/apache/sqoop/tool/CodeGenTool.java PRE-CREATION > /src/java/org/apache/sqoop/tool/CreateHiveTableTool.java PRE-CREATION > /src/java/org/apache/sqoop/tool/EvalSqlTool.java PRE-CREATION > /src/java/org/apache/sqoop/tool/ExportTool.java PRE-CREATION > /src/java/org/apache/sqoop/tool/HelpTool.java PRE-CREATION > /src/java/org/apache/sqoop/tool/ImportAllTablesTool.java PRE-CREATION > /src/java/org/apache/sqoop/tool/ImportTool.java PRE-CREATION > /src/java/org/apache/sqoop/tool/JobTool.java PRE-CREATION > /src/java/org/apache/sqoop/tool/ListDatabasesTool.java PRE-CREATION > /src/java/org/apache/sqoop/tool/ListTablesTool.java PRE-CREATION > /src/java/org/apache/sqoop/tool/MergeTool.java PRE-CREATION > /src/java/org/apache/sqoop/tool/MetastoreTool.java PRE-CREATION > /src/java/org/apache/sqoop/tool/SqoopTool.java PRE-CREATION > /src/java/org/apache/sqoop/tool/ToolDesc.java PRE-CREATION > /src/java/org/apache/sqoop/tool/ToolPlugin.java PRE-CREATION > /src/java/org/apache/sqoop/tool/VersionTool.java PRE-CREATION > /src/test/com/cloudera/sqoop/orm/TestClassWriter.java 1190173 > /src/test/com/cloudera/sqoop/orm/TestParseMethods.java 1190173 > /src/test/com/cloudera/sqoop/tool/TestToolPlugin.java 1190173 > > Diff: https://reviews.apache.org/r/2574/diff > > > Testing > ------- > > > Thanks, > > Jarek > >
