----------------------------------------------------------- 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 > >
