----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33708/#review82159 -----------------------------------------------------------
Ship it! Good stuff Abe! I'm +1 on the change (it already passed pre-commit hook). I do have few nits, mostly for comments: core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java <https://reviews.apache.org/r/33708/#comment132848> Can we alter the comment a bit to make it more clear what we're doing? Something like: "Adding jars to current classloader from property:" core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java <https://reviews.apache.org/r/33708/#comment132849> Can we alter the comment a bit to make it more clear what we're doing? Something like: "Property #{classpathPropety} is null or empty. Not adding any extra jars." core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java <https://reviews.apache.org/r/33708/#comment132853> Can we alter the comment a bit to make it more clear what we're doing? Something like: "Found jar in path:" core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java <https://reviews.apache.org/r/33708/#comment132854> Shouldn't we print here only one URL and not entire List? core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java <https://reviews.apache.org/r/33708/#comment132851> Nit: I'm thinking if this log output still have a value? docs/src/site/sphinx/ConnectorDevelopment.rst <https://reviews.apache.org/r/33708/#comment132856> I believe that with this change, we're removing the explicit directory listing that we were doing before. Hence we should update this comment to specify that user needs to configure the path to the jar itself (and not "to this folder"). docs/src/site/sphinx/ConnectorDevelopment.rst <https://reviews.apache.org/r/33708/#comment132857> I would update this comment with copy&paste from the sqoop.properties file as well. - Jarek Cecho On April 30, 2015, 8:13 a.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33708/ > ----------------------------------------------------------- > > (Updated April 30, 2015, 8:13 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2342 > https://issues.apache.org/jira/browse/SQOOP-2342 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > commit 85fd8cc9826207f94d14645fbcfec8a5b37102da > Author: Abraham Elmahrek <[email protected]> > Date: Wed Apr 29 17:17:27 2015 -0700 > > SQOOP-2342: Sqoop2: Provide an external application classloader > > :100644 100644 24e73c3... 4690a30... M > common/src/main/java/org/apache/sqoop/error/code/CoreError.java > :100644 100644 49e5d6f... a0e4984... M > core/src/main/java/org/apache/sqoop/core/ConfigurationConstants.java > :100644 100644 15ee12f... ff9a29e... M > core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java > :100755 100755 5226a19... 424d3cb... M > dist/src/main/server/conf/sqoop.properties > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/error/code/CoreError.java 24e73c3 > core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java 0517413 > core/src/main/java/org/apache/sqoop/connector/ConnectorManagerUtils.java > 99736c6 > core/src/main/java/org/apache/sqoop/core/ConfigurationConstants.java > 49e5d6f > core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java 15ee12f > > core/src/test/java/org/apache/sqoop/connector/TestConnectorManagerUtils.java > f2e64cd > core/src/test/java/org/apache/sqoop/core/TestSqoopConfiguration.java > b11587c > core/src/test/resources/test_config.properties 4ad1267 > dist/src/main/server/conf/sqoop.properties 5226a19 > docs/src/site/sphinx/ConnectorDevelopment.rst 2d9e7d2 > test/src/main/java/org/apache/sqoop/test/minicluster/SqoopMiniCluster.java > ad45189 > > Diff: https://reviews.apache.org/r/33708/diff/ > > > Testing > ------- > > manually tested that new jars are being loaded. > > > Thanks, > > Abraham Elmahrek > >
