----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33708/#review82070 -----------------------------------------------------------
common/src/main/java/org/apache/sqoop/error/code/CoreError.java <https://reviews.apache.org/r/33708/#comment132733> Nit: Can we end the statement with comma,so that we don't have to change this line next time we'll be adding new error code? core/src/main/java/org/apache/sqoop/core/ConfigurationConstants.java <https://reviews.apache.org/r/33708/#comment132752> It seems that we have similar functionality of loading external jars for connector already (via this property). Do you think that it would make sense to merge those two ways together? And perhaps just have "this is extra classpath" and don't really care whether it's connector or additional libraries. core/src/main/java/org/apache/sqoop/core/ConfigurationConstants.java <https://reviews.apache.org/r/33708/#comment132748> I'm a bit concerned that the name will be interpreted as "this is Sqoop server class path" when in reality it's about extra jars that should be put on the classpath. Would you be open to rename it? Something like "org.sqoop.classpath.extra" or ".additional" or something like that. core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java <https://reviews.apache.org/r/33708/#comment132749> Nit for the comments inside this method - can we make obvious that we're working with "extra" classpath. E.g. that we're not changing the existing one, but that we're adding new stuff to it. core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java <https://reviews.apache.org/r/33708/#comment132750> Nit: Let's perahps move this to the for loop, so that we get each jar printed out? dist/src/main/server/conf/sqoop.properties <https://reviews.apache.org/r/33708/#comment132751> I believe that the URLClassLoader doesn't accept directories with jars - it expects every jar explicitly enumerated. If I'm not mistaken, then the directories are considering as complied code and hence they will be searched only for .class files without looking into any archives. - Jarek Cecho On April 30, 2015, 12:21 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, 12:21 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/core/ConfigurationConstants.java > 49e5d6f > core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java 15ee12f > dist/src/main/server/conf/sqoop.properties 5226a19 > > Diff: https://reviews.apache.org/r/33708/diff/ > > > Testing > ------- > > manually tested that new jars are being loaded. > > > Thanks, > > Abraham Elmahrek > >
