-----------------------------------------------------------
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:
NEW: Monitor These Apps!
elasticsearch, apache solr, apache hbase, hadoop, redis, casssandra, amazon cloudwatch, mysql, memcached, apache kafka, apache zookeeper, apache storm, ubuntu, centOS, red hat, debian, puppet labs, java, senseiDB