----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39030/#review101702 -----------------------------------------------------------
Few comments: core/src/main/java/org/apache/sqoop/core/ConfigurationConstants.java (line 87) <https://reviews.apache.org/r/39030/#comment159159> Nit: Might I suggest to rename this variable to org.apache.sqoop.classpath.job or something. So that it's in the same "prefix" as the property CLASSPATH. core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java (lines 280 - 310) <https://reviews.apache.org/r/39030/#comment159163> I somehow feel that the job of parsing the configuration property shouldn't be here but rather in the JobManager class. The SqoopConfiguration is a general class that in general doesn't know about semantics of underlaying properties (there are few exceptions though) and the semantics is given in the code that is actually using the configuration propertly. Good example of that is the property org.apache.sqoop.classpath.extra that also contains extra classpath entires, but yet we're not parsing them in this class. core/src/main/java/org/apache/sqoop/driver/configuration/JarConfig.java (lines 26 - 30) <https://reviews.apache.org/r/39030/#comment159164> You will need to create entries in driver-config.properties: https://github.com/apache/sqoop/blob/sqoop2/core/src/main/resources/driver-config.properties Might I suggest to port this tescase to Driver as well? https://github.com/apache/sqoop/blob/sqoop2/connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestGenericJdbcConnector.java The test case would uncover this missing piece. core/src/main/java/org/apache/sqoop/driver/configuration/JarConfig.java (line 28) <https://reviews.apache.org/r/39030/#comment159160> Super nit: Please put space between the "//" and "A" dist/pom.xml (line 194) <https://reviews.apache.org/r/39030/#comment159161> This change doesn't seem to be relevant to this patch, so let's nuke it? Jarcec - Jarek Cecho On Oct. 6, 2015, 7:48 p.m., Abraham Fine wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39030/ > ----------------------------------------------------------- > > (Updated Oct. 6, 2015, 7:48 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2490 > https://issues.apache.org/jira/browse/SQOOP-2490 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > Sqoop2: Add extra jars to job > > > Diffs > ----- > > core/src/main/java/org/apache/sqoop/core/ConfigurationConstants.java > d7fe27b > core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java 52e432d > core/src/main/java/org/apache/sqoop/driver/JobManager.java ebb7efd > core/src/main/java/org/apache/sqoop/driver/configuration/JarConfig.java > PRE-CREATION > > core/src/main/java/org/apache/sqoop/driver/configuration/JobConfiguration.java > bf1328a > core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java f9649f2 > dist/pom.xml ff18487 > dist/src/main/server/conf/sqoop.properties fe8bcce > test/src/main/java/org/apache/sqoop/test/minicluster/SqoopMiniCluster.java > 7440025 > > test/src/test/java/org/apache/sqoop/integration/classpath/ClasspathTest.java > PRE-CREATION > test/src/test/resources/TestConnector/TestConnector.java PRE-CREATION > test/src/test/resources/TestConnector/TestDependency.java PRE-CREATION > test/src/test/resources/TestConnector/TestLinkConfiguration.java > PRE-CREATION > test/src/test/resources/TestConnector/TestLoader.java PRE-CREATION > test/src/test/resources/TestConnector/TestToDestroyer.java PRE-CREATION > test/src/test/resources/TestConnector/TestToInitializer.java PRE-CREATION > test/src/test/resources/TestConnector/TestToJobConfiguration.java > PRE-CREATION > test/src/test/resources/TestConnector/sqoopconnector.properties > PRE-CREATION > test/src/test/resources/classpath-tests-suite.xml PRE-CREATION > > Diff: https://reviews.apache.org/r/39030/diff/ > > > Testing > ------- > > yes > > > Thanks, > > Abraham Fine > >
