You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Dian Fu <di...@gmail.com> on 2015/10/16 15:38:54 UTC
Review Request 39396: Sqoop2: Provide classpath isolation for
Connectors
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39396/
-----------------------------------------------------------
Review request for Sqoop.
Bugs: SQOOP-2621
https://issues.apache.org/jira/browse/SQOOP-2621
Repository: sqoop-sqoop2
Description
-------
Provide classpath isolation for connectors and also integration test.
Diffs
-----
common/src/main/java/org/apache/sqoop/error/code/ConnectorError.java 2f17d95
common/src/main/java/org/apache/sqoop/utils/ClassUtils.java ed68988
common/src/main/java/org/apache/sqoop/utils/ContextUtils.java 25d6fcb
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnector.java 113465a
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnector.java 7e7c022
connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConnector.java ca860b1
connector/connector-sdk/src/main/java/org/apache/sqoop/connector/spi/SqoopConnector.java 85ba8be
connector/connector-sdk/src/main/java/org/apache/sqoop/job/etl/From.java e9d2abe
connector/connector-sdk/src/main/java/org/apache/sqoop/job/etl/Partition.java c4664a6
core/src/main/java/org/apache/sqoop/connector/ConnectorHandler.java 1899bb7
core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java 285e893
core/src/main/java/org/apache/sqoop/connector/ConnectorManagerUtils.java 522ed08
core/src/main/java/org/apache/sqoop/connector/ConnectorUtils.java PRE-CREATION
core/src/main/java/org/apache/sqoop/core/ConfigurationConstants.java f49fca3
core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java 3c2f67e
core/src/main/java/org/apache/sqoop/driver/JobManager.java 0d230f9
core/src/main/java/org/apache/sqoop/driver/JobRequest.java cfa45b2
core/src/test/java/org/apache/sqoop/connector/TestConnectorManagerUtils.java fd2d1b4
core/src/test/java/org/apache/sqoop/connector/TestConnectorUtils.java PRE-CREATION
dist/src/main/conf/sqoop.properties 7c1ec18
execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java c8d210e
execution/mapreduce/src/main/java/org/apache/sqoop/job/MRJobConstants.java b7aa8c6
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java 1e1b237
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRUtils.java PRE-CREATION
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java b3c1ce8
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 67189a1
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 937ef5a
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopNullOutputFormat.java 88ab98e
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java d94b658
execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2463643
execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMatching.java d0b41d1
execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java 6a12d47
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 0302150
submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java f396783
test/src/main/java/org/apache/sqoop/test/testcases/TestClasspathBase.java PRE-CREATION
test/src/test/java/org/apache/sqoop/integration/classpath/ClasspathTest.java 393b800
test/src/test/java/org/apache/sqoop/integration/classpath/ConnectorClasspathIsolationTest.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/TestClasspathIsolation.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/TestExtractor.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromConnector.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromDestroyer.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromInitializer.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromJobConfiguration.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromLinkConfiguration.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/TestPartition.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/TestPartitioner.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/sqoopconnector.properties PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/to/TestClasspathIsolation.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/to/TestLoader.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/to/TestToConnector.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/to/TestToDestroyer.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/to/TestToInitializer.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/to/TestToJobConfiguration.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/to/TestToLinkConfiguration.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/to/sqoopconnector.properties PRE-CREATION
Diff: https://reviews.apache.org/r/39396/diff/
Testing
-------
Thanks,
Dian Fu
Re: Review Request 39396: Sqoop2: Provide classpath isolation for
Connectors
Posted by Jarek Cecho <ja...@apache.org>.
> On Oct. 21, 2015, 5:37 p.m., Jarek Cecho wrote:
> > dist/src/main/conf/sqoop.properties, lines 180-188
> > <https://reviews.apache.org/r/39396/diff/2/?file=1100699#file1100699line180>
> >
> > The goal of this property is to enable to load connectors from path external to Sqoop server usual directories. We should retain this functionality.
> >
> > The use case is to for example configure this to /opt/sqoop/connectors so that admin can install any third party connector without the need to mess with Sqoop 2 instalation directories.
>
> Dian Fu wrote:
> SQOOP-2342 has added property "org.apache.sqoop.classpath.extra" and dropped the use of property "org.apache.sqoop.connector.external.loadpath". Seems that property "org.apache.sqoop.classpath.extra" is a rename of property "org.apache.sqoop.connector.external.loadpath". Currently property "org.apache.sqoop.connector.external.loadpath" isn't used at all.
Good point, then we should probably remove that property from the default configuration file :)
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39396/#review103428
-----------------------------------------------------------
On Oct. 19, 2015, 3:03 a.m., Dian Fu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39396/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2015, 3:03 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2621
> https://issues.apache.org/jira/browse/SQOOP-2621
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Provide classpath isolation for connectors and also integration test.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/error/code/ConnectorError.java 2f17d95
> common/src/main/java/org/apache/sqoop/utils/ClassUtils.java ed68988
> common/src/main/java/org/apache/sqoop/utils/ContextUtils.java 25d6fcb
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnector.java 113465a
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnector.java 7e7c022
> connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConnector.java ca860b1
> connector/connector-sdk/src/main/java/org/apache/sqoop/connector/spi/SqoopConnector.java 85ba8be
> connector/connector-sdk/src/main/java/org/apache/sqoop/job/etl/From.java e9d2abe
> connector/connector-sdk/src/main/java/org/apache/sqoop/job/etl/Partition.java c4664a6
> core/src/main/java/org/apache/sqoop/connector/ConnectorHandler.java 1899bb7
> core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java 285e893
> core/src/main/java/org/apache/sqoop/connector/ConnectorManagerUtils.java 522ed08
> core/src/main/java/org/apache/sqoop/connector/ConnectorUtils.java PRE-CREATION
> core/src/main/java/org/apache/sqoop/core/ConfigurationConstants.java f49fca3
> core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java 3c2f67e
> core/src/main/java/org/apache/sqoop/driver/JobManager.java 0d230f9
> core/src/main/java/org/apache/sqoop/driver/JobRequest.java cfa45b2
> core/src/test/java/org/apache/sqoop/connector/TestConnectorManagerUtils.java fd2d1b4
> core/src/test/java/org/apache/sqoop/connector/TestConnectorUtils.java PRE-CREATION
> dist/src/main/conf/sqoop.properties 7c1ec18
> execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java c8d210e
> execution/mapreduce/src/main/java/org/apache/sqoop/job/MRJobConstants.java b7aa8c6
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java 1e1b237
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRUtils.java PRE-CREATION
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java b3c1ce8
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 67189a1
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 937ef5a
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopNullOutputFormat.java 88ab98e
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java d94b658
> execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2463643
> execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMatching.java d0b41d1
> execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java 6a12d47
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 0302150
> submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java f396783
> test/src/main/java/org/apache/sqoop/test/testcases/TestClasspathBase.java PRE-CREATION
> test/src/test/java/org/apache/sqoop/integration/classpath/ClasspathTest.java 393b800
> test/src/test/java/org/apache/sqoop/integration/classpath/ConnectorClasspathIsolationTest.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestClasspathIsolation.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestExtractor.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromConnector.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromDestroyer.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromInitializer.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromJobConfiguration.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromLinkConfiguration.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestPartition.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestPartitioner.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/sqoopconnector.properties PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestClasspathIsolation.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestLoader.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestToConnector.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestToDestroyer.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestToInitializer.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestToJobConfiguration.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestToLinkConfiguration.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/sqoopconnector.properties PRE-CREATION
>
> Diff: https://reviews.apache.org/r/39396/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dian Fu
>
>
Re: Review Request 39396: Sqoop2: Provide classpath isolation for
Connectors
Posted by Dian Fu <di...@gmail.com>.
> On Oct. 21, 2015, 5:37 p.m., Jarek Cecho wrote:
> > Thank you for pushing this one forward Dian! I've preliminary looked at the patch and left some comments that immediately caught my eye.
> >
> > The patch is quite huge and it seems to cover a lot of changes, so I would like to propose splitting it into independent pieces. This will make both the review a bit smoother (+ faster) and will enable us to track each of the indepenent changes separately (git blame and git cherrypick will work better as well). Quickly looking it seems that the course of action should be something in the lines of:
> >
> > 1) We should make the required changes to the connector interfaces (adding partition class to the FROM/TO) and propagating it to the execution engine.
> >
> > 2) We should redesign way that connectors are expressing jar dependencies. I feel that this is a big topic that by itself should be covered by design document iterating over options we have and suggesting the approach we should take.
> >
> > 3) Make the connector classes to be actually loaded with different classloader.
> >
> > What do you think?
Strongly agree with you to split the patch. Will do that.
> On Oct. 21, 2015, 5:37 p.m., Jarek Cecho wrote:
> > connector/connector-sdk/src/main/java/org/apache/sqoop/job/etl/From.java, line 34
> > <https://reviews.apache.org/r/39396/diff/2/?file=1100687#file1100687line34>
> >
> > Can we please make the changes to Connector interface in separate JIRA?
Good advice. Have filed ticket SQOOP-2632 to cover the changes of Connector interface.
> On Oct. 21, 2015, 5:37 p.m., Jarek Cecho wrote:
> > common/src/main/java/org/apache/sqoop/error/code/ConnectorError.java, line 65
> > <https://reviews.apache.org/r/39396/diff/2/?file=1100680#file1100680line65>
> >
> > Nit: Can we please add comma at the end of the line and put the semilcolon in separate line. The fact that we have to change previous line to add new error code messes with "git blame" command.
Good advice. Will do that. :)
> On Oct. 21, 2015, 5:37 p.m., Jarek Cecho wrote:
> > dist/src/main/conf/sqoop.properties, lines 180-188
> > <https://reviews.apache.org/r/39396/diff/2/?file=1100699#file1100699line180>
> >
> > The goal of this property is to enable to load connectors from path external to Sqoop server usual directories. We should retain this functionality.
> >
> > The use case is to for example configure this to /opt/sqoop/connectors so that admin can install any third party connector without the need to mess with Sqoop 2 instalation directories.
SQOOP-2342 has added property "org.apache.sqoop.classpath.extra" and dropped the use of property "org.apache.sqoop.connector.external.loadpath". Seems that property "org.apache.sqoop.classpath.extra" is a rename of property "org.apache.sqoop.connector.external.loadpath". Currently property "org.apache.sqoop.connector.external.loadpath" isn't used at all.
- Dian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39396/#review103428
-----------------------------------------------------------
On Oct. 19, 2015, 3:03 a.m., Dian Fu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39396/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2015, 3:03 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2621
> https://issues.apache.org/jira/browse/SQOOP-2621
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Provide classpath isolation for connectors and also integration test.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/error/code/ConnectorError.java 2f17d95
> common/src/main/java/org/apache/sqoop/utils/ClassUtils.java ed68988
> common/src/main/java/org/apache/sqoop/utils/ContextUtils.java 25d6fcb
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnector.java 113465a
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnector.java 7e7c022
> connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConnector.java ca860b1
> connector/connector-sdk/src/main/java/org/apache/sqoop/connector/spi/SqoopConnector.java 85ba8be
> connector/connector-sdk/src/main/java/org/apache/sqoop/job/etl/From.java e9d2abe
> connector/connector-sdk/src/main/java/org/apache/sqoop/job/etl/Partition.java c4664a6
> core/src/main/java/org/apache/sqoop/connector/ConnectorHandler.java 1899bb7
> core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java 285e893
> core/src/main/java/org/apache/sqoop/connector/ConnectorManagerUtils.java 522ed08
> core/src/main/java/org/apache/sqoop/connector/ConnectorUtils.java PRE-CREATION
> core/src/main/java/org/apache/sqoop/core/ConfigurationConstants.java f49fca3
> core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java 3c2f67e
> core/src/main/java/org/apache/sqoop/driver/JobManager.java 0d230f9
> core/src/main/java/org/apache/sqoop/driver/JobRequest.java cfa45b2
> core/src/test/java/org/apache/sqoop/connector/TestConnectorManagerUtils.java fd2d1b4
> core/src/test/java/org/apache/sqoop/connector/TestConnectorUtils.java PRE-CREATION
> dist/src/main/conf/sqoop.properties 7c1ec18
> execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java c8d210e
> execution/mapreduce/src/main/java/org/apache/sqoop/job/MRJobConstants.java b7aa8c6
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java 1e1b237
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRUtils.java PRE-CREATION
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java b3c1ce8
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 67189a1
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 937ef5a
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopNullOutputFormat.java 88ab98e
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java d94b658
> execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2463643
> execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMatching.java d0b41d1
> execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java 6a12d47
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 0302150
> submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java f396783
> test/src/main/java/org/apache/sqoop/test/testcases/TestClasspathBase.java PRE-CREATION
> test/src/test/java/org/apache/sqoop/integration/classpath/ClasspathTest.java 393b800
> test/src/test/java/org/apache/sqoop/integration/classpath/ConnectorClasspathIsolationTest.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestClasspathIsolation.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestExtractor.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromConnector.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromDestroyer.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromInitializer.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromJobConfiguration.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromLinkConfiguration.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestPartition.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestPartitioner.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/sqoopconnector.properties PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestClasspathIsolation.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestLoader.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestToConnector.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestToDestroyer.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestToInitializer.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestToJobConfiguration.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestToLinkConfiguration.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/sqoopconnector.properties PRE-CREATION
>
> Diff: https://reviews.apache.org/r/39396/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dian Fu
>
>
Re: Review Request 39396: Sqoop2: Provide classpath isolation for
Connectors
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39396/#review103428
-----------------------------------------------------------
Thank you for pushing this one forward Dian! I've preliminary looked at the patch and left some comments that immediately caught my eye.
The patch is quite huge and it seems to cover a lot of changes, so I would like to propose splitting it into independent pieces. This will make both the review a bit smoother (+ faster) and will enable us to track each of the indepenent changes separately (git blame and git cherrypick will work better as well). Quickly looking it seems that the course of action should be something in the lines of:
1) We should make the required changes to the connector interfaces (adding partition class to the FROM/TO) and propagating it to the execution engine.
2) We should redesign way that connectors are expressing jar dependencies. I feel that this is a big topic that by itself should be covered by design document iterating over options we have and suggesting the approach we should take.
3) Make the connector classes to be actually loaded with different classloader.
What do you think?
common/src/main/java/org/apache/sqoop/error/code/ConnectorError.java (line 65)
<https://reviews.apache.org/r/39396/#comment161481>
Nit: Can we please add comma at the end of the line and put the semilcolon in separate line. The fact that we have to change previous line to add new error code messes with "git blame" command.
connector/connector-sdk/src/main/java/org/apache/sqoop/job/etl/From.java (line 34)
<https://reviews.apache.org/r/39396/#comment161482>
Can we please make the changes to Connector interface in separate JIRA?
dist/src/main/conf/sqoop.properties (lines 180 - 186)
<https://reviews.apache.org/r/39396/#comment161490>
The goal of this property is to enable to load connectors from path external to Sqoop server usual directories. We should retain this functionality.
The use case is to for example configure this to /opt/sqoop/connectors so that admin can install any third party connector without the need to mess with Sqoop 2 instalation directories.
- Jarek Cecho
On Oct. 19, 2015, 3:03 a.m., Dian Fu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39396/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2015, 3:03 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2621
> https://issues.apache.org/jira/browse/SQOOP-2621
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Provide classpath isolation for connectors and also integration test.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/error/code/ConnectorError.java 2f17d95
> common/src/main/java/org/apache/sqoop/utils/ClassUtils.java ed68988
> common/src/main/java/org/apache/sqoop/utils/ContextUtils.java 25d6fcb
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnector.java 113465a
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnector.java 7e7c022
> connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConnector.java ca860b1
> connector/connector-sdk/src/main/java/org/apache/sqoop/connector/spi/SqoopConnector.java 85ba8be
> connector/connector-sdk/src/main/java/org/apache/sqoop/job/etl/From.java e9d2abe
> connector/connector-sdk/src/main/java/org/apache/sqoop/job/etl/Partition.java c4664a6
> core/src/main/java/org/apache/sqoop/connector/ConnectorHandler.java 1899bb7
> core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java 285e893
> core/src/main/java/org/apache/sqoop/connector/ConnectorManagerUtils.java 522ed08
> core/src/main/java/org/apache/sqoop/connector/ConnectorUtils.java PRE-CREATION
> core/src/main/java/org/apache/sqoop/core/ConfigurationConstants.java f49fca3
> core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java 3c2f67e
> core/src/main/java/org/apache/sqoop/driver/JobManager.java 0d230f9
> core/src/main/java/org/apache/sqoop/driver/JobRequest.java cfa45b2
> core/src/test/java/org/apache/sqoop/connector/TestConnectorManagerUtils.java fd2d1b4
> core/src/test/java/org/apache/sqoop/connector/TestConnectorUtils.java PRE-CREATION
> dist/src/main/conf/sqoop.properties 7c1ec18
> execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java c8d210e
> execution/mapreduce/src/main/java/org/apache/sqoop/job/MRJobConstants.java b7aa8c6
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java 1e1b237
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRUtils.java PRE-CREATION
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java b3c1ce8
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 67189a1
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 937ef5a
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopNullOutputFormat.java 88ab98e
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java d94b658
> execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2463643
> execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMatching.java d0b41d1
> execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java 6a12d47
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 0302150
> submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java f396783
> test/src/main/java/org/apache/sqoop/test/testcases/TestClasspathBase.java PRE-CREATION
> test/src/test/java/org/apache/sqoop/integration/classpath/ClasspathTest.java 393b800
> test/src/test/java/org/apache/sqoop/integration/classpath/ConnectorClasspathIsolationTest.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestClasspathIsolation.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestExtractor.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromConnector.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromDestroyer.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromInitializer.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromJobConfiguration.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromLinkConfiguration.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestPartition.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/TestPartitioner.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/from/sqoopconnector.properties PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestClasspathIsolation.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestLoader.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestToConnector.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestToDestroyer.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestToInitializer.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestToJobConfiguration.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/TestToLinkConfiguration.java PRE-CREATION
> test/src/test/resources/TestConnectorClasspathIsolation/to/sqoopconnector.properties PRE-CREATION
>
> Diff: https://reviews.apache.org/r/39396/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dian Fu
>
>
Re: Review Request 39396: Sqoop2: Provide classpath isolation for
Connectors
Posted by Dian Fu <di...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39396/
-----------------------------------------------------------
(Updated Oct. 19, 2015, 3:03 a.m.)
Review request for Sqoop.
Bugs: SQOOP-2621
https://issues.apache.org/jira/browse/SQOOP-2621
Repository: sqoop-sqoop2
Description
-------
Provide classpath isolation for connectors and also integration test.
Diffs (updated)
-----
common/src/main/java/org/apache/sqoop/error/code/ConnectorError.java 2f17d95
common/src/main/java/org/apache/sqoop/utils/ClassUtils.java ed68988
common/src/main/java/org/apache/sqoop/utils/ContextUtils.java 25d6fcb
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnector.java 113465a
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnector.java 7e7c022
connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConnector.java ca860b1
connector/connector-sdk/src/main/java/org/apache/sqoop/connector/spi/SqoopConnector.java 85ba8be
connector/connector-sdk/src/main/java/org/apache/sqoop/job/etl/From.java e9d2abe
connector/connector-sdk/src/main/java/org/apache/sqoop/job/etl/Partition.java c4664a6
core/src/main/java/org/apache/sqoop/connector/ConnectorHandler.java 1899bb7
core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java 285e893
core/src/main/java/org/apache/sqoop/connector/ConnectorManagerUtils.java 522ed08
core/src/main/java/org/apache/sqoop/connector/ConnectorUtils.java PRE-CREATION
core/src/main/java/org/apache/sqoop/core/ConfigurationConstants.java f49fca3
core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java 3c2f67e
core/src/main/java/org/apache/sqoop/driver/JobManager.java 0d230f9
core/src/main/java/org/apache/sqoop/driver/JobRequest.java cfa45b2
core/src/test/java/org/apache/sqoop/connector/TestConnectorManagerUtils.java fd2d1b4
core/src/test/java/org/apache/sqoop/connector/TestConnectorUtils.java PRE-CREATION
dist/src/main/conf/sqoop.properties 7c1ec18
execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java c8d210e
execution/mapreduce/src/main/java/org/apache/sqoop/job/MRJobConstants.java b7aa8c6
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java 1e1b237
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRUtils.java PRE-CREATION
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java b3c1ce8
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 67189a1
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 937ef5a
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopNullOutputFormat.java 88ab98e
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java d94b658
execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2463643
execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMatching.java d0b41d1
execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java 6a12d47
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 0302150
submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java f396783
test/src/main/java/org/apache/sqoop/test/testcases/TestClasspathBase.java PRE-CREATION
test/src/test/java/org/apache/sqoop/integration/classpath/ClasspathTest.java 393b800
test/src/test/java/org/apache/sqoop/integration/classpath/ConnectorClasspathIsolationTest.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/TestClasspathIsolation.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/TestExtractor.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromConnector.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromDestroyer.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromInitializer.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromJobConfiguration.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/TestFromLinkConfiguration.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/TestPartition.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/TestPartitioner.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/from/sqoopconnector.properties PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/to/TestClasspathIsolation.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/to/TestLoader.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/to/TestToConnector.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/to/TestToDestroyer.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/to/TestToInitializer.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/to/TestToJobConfiguration.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/to/TestToLinkConfiguration.java PRE-CREATION
test/src/test/resources/TestConnectorClasspathIsolation/to/sqoopconnector.properties PRE-CREATION
Diff: https://reviews.apache.org/r/39396/diff/
Testing
-------
Thanks,
Dian Fu