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 Elmahrek <ab...@cloudera.com> on 2015/04/30 02:21:58 UTC
Review Request 33708: SQOOP-2342: Sqoop2: Provide an external
application classloader
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33708/
-----------------------------------------------------------
Review request for Sqoop.
Bugs: SQOOP-2342
https://issues.apache.org/jira/browse/SQOOP-2342
Repository: sqoop-sqoop2
Description
-------
commit 85fd8cc9826207f94d14645fbcfec8a5b37102da
Author: Abraham Elmahrek <ab...@apache.org>
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
Re: Review Request 33708: SQOOP-2342: Sqoop2: Provide an external
application classloader
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
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 <ab...@apache.org>
> 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
>
>
Re: Review Request 33708: SQOOP-2342: Sqoop2: Provide an external
application classloader
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33708/#review82202
-----------------------------------------------------------
Ship it!
Ship It!
- Jarek Cecho
On April 30, 2015, 5:42 p.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33708/
> -----------------------------------------------------------
>
> (Updated April 30, 2015, 5:42 p.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 <ab...@apache.org>
> 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
>
>
Re: Review Request 33708: SQOOP-2342: Sqoop2: Provide an external
application classloader
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33708/
-----------------------------------------------------------
(Updated April 30, 2015, 5:42 p.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 <ab...@apache.org>
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 (updated)
-----
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
Re: Review Request 33708: SQOOP-2342: Sqoop2: Provide an external
application classloader
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
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.
Changes
-------
Added test
Bugs: SQOOP-2342
https://issues.apache.org/jira/browse/SQOOP-2342
Repository: sqoop-sqoop2
Description
-------
commit 85fd8cc9826207f94d14645fbcfec8a5b37102da
Author: Abraham Elmahrek <ab...@apache.org>
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 (updated)
-----
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
Re: Review Request 33708: SQOOP-2342: Sqoop2: Provide an external
application classloader
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33708/
-----------------------------------------------------------
(Updated April 30, 2015, 1:55 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 <ab...@apache.org>
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 (updated)
-----
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
dist/src/main/server/conf/sqoop.properties 5226a19
docs/src/site/sphinx/ConnectorDevelopment.rst 2d9e7d2
Diff: https://reviews.apache.org/r/33708/diff/
Testing
-------
manually tested that new jars are being loaded.
Thanks,
Abraham Elmahrek
Re: Review Request 33708: SQOOP-2342: Sqoop2: Provide an external
application classloader
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> On April 30, 2015, 1:42 a.m., Jarek Cecho wrote:
> > dist/src/main/server/conf/sqoop.properties, lines 175-178
> > <https://reviews.apache.org/r/33708/diff/1/?file=945994#file945994line175>
> >
> > 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.
Just making this comment specific to jars.
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33708/#review82070
-----------------------------------------------------------
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 <ab...@apache.org>
> 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
>
>
Re: Review Request 33708: SQOOP-2342: Sqoop2: Provide an external
application classloader
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
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 <ab...@apache.org>
> 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
>
>