You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Abraham Fine <ab...@brightroll.com> on 2015/10/06 01:28:02 UTC
Review Request 39030: SQOOP-2490
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39030/
-----------------------------------------------------------
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
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/server/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
Diff: https://reviews.apache.org/r/39030/diff/
Testing
-------
yes
Thanks,
Abraham Fine
Re: Review Request 39030: SQOOP-2490
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39030/#review101728
-----------------------------------------------------------
Ship it!
+1 as long as precommit hook passes
- Jarek Cecho
On Oct. 7, 2015, 12:05 a.m., Abraham Fine wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39030/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2015, 12:05 a.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
> core/src/main/resources/driver-config.properties e005775
> core/src/test/java/org/apache/sqoop/driver/TestJobConfiguration.java PRE-CREATION
> 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
>
>
Re: Review Request 39030: SQOOP-2490
Posted by Abraham Fine <ab...@brightroll.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39030/
-----------------------------------------------------------
(Updated Oct. 7, 2015, 12:05 a.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 (updated)
-----
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
core/src/main/resources/driver-config.properties e005775
core/src/test/java/org/apache/sqoop/driver/TestJobConfiguration.java PRE-CREATION
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
Re: Review Request 39030: SQOOP-2490
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 39030: SQOOP-2490
Posted by Abraham Fine <ab...@brightroll.com>.
-----------------------------------------------------------
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 (updated)
-----
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