----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39586/#review108543 -----------------------------------------------------------
My apologies for jumping so late on this review. Few high level notes: 1) It seems that the patch no longer applies cleanly, so you will have to rebase it. 2) I know that we've changed the design quite a bit - would you mind updating the JIRA and the review board summary with what exactly we're trying to accomplish here? I don't think that the title "Sqoop2: Allow connectors to express jar dependencies" is still relevant. It's more about wrap connector's dependencies inside the connector's jar, right? 3) This is rather huge change - I'm wondering if there are independent pieces (like changes to ConnectorClassLoader) that can be submitted via independent JIRA? It will make the subsequent reviews easier. And few more tactical: common/src/main/resources/org.apache.sqoop.connector-classloader.properties (line 54) <https://reviews.apache.org/r/39586/#comment167983> Wondering what is the "-" at the begging of the line mean? connector/connector-ftp/pom.xml (line 80) <https://reviews.apache.org/r/39586/#comment167985> Can we define the assembly plugin in root pom.xml so that we don't have repeat the version every time in each children pom file? We do that alredy for pretty much all dependencies (there are few exceptions) and it's much cleaner. connector/connector-ftp/pom.xml (lines 88 - 102) <https://reviews.apache.org/r/39586/#comment167986> Wondering if there is a way to somehow put this fragment to some of the parent pom.xml files so that we don't have to repeat it every time? Perhaps we can create new parent module "sqoop-connector" that will contain the packaging portion and use it for all children? connector/connector-ftp/pom.xml (line 96) <https://reviews.apache.org/r/39586/#comment167987> Let's not hardcode the project name here - it will be hard to mantain and prone to errors. We should be able to get the project name from ${project.artifactId} or similar variable. connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteFromInitializer.java (lines 60 - 68) <https://reviews.apache.org/r/39586/#comment167988> Aren't those dependecies on inside the connector's jar as well? core/src/main/java/org/apache/sqoop/connector/ConnectorHandler.java (lines 74 - 76) <https://reviews.apache.org/r/39586/#comment168502> Can we abstract this "magic" to standalone method and provide proper test coverage? It seems fragile, so having good tests will hopefully help ensuring that we won't break this in the future. core/src/main/java/org/apache/sqoop/connector/ConnectorManagerUtils.java (line 54) <https://reviews.apache.org/r/39586/#comment168503> Wondering why do we need to keep the classloader dependencies around? Can't we just store the classloder instance inside the ConnectorHandler or perhaps even easier call connectorInstance.getClass().getClassLoader(). core/src/main/java/org/apache/sqoop/connector/ConnectorManagerUtils.java (lines 183 - 199) <https://reviews.apache.org/r/39586/#comment168504> I'm wondering if there is a way to avoid creating the intermediate files on disk? How are other similar libraries solving (One jar for example) the same problem? core/src/main/java/org/apache/sqoop/driver/JobManager.java (lines 490 - 494) <https://reviews.apache.org/r/39586/#comment168505> I'm wondering if this is needed? Based on preliminary research any "new" call inside the initializer() method should use the Connector classes classloader. execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java (line 86) <https://reviews.apache.org/r/39586/#comment168506> We're not really storing the classpath here, just the jar location. Also I'm wondering if this is really desirable to store since the path is absolute on given machine, right? execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java (lines 61 - 78) <https://reviews.apache.org/r/39586/#comment168507> I think that easier and more nicer way how to do this is to load the SqoopConnector class with given class loader. Everything else we should "get" for free. server/pom.xml (lines 83 - 88) <https://reviews.apache.org/r/39586/#comment168508> I'm wondering why do we need the exclusions? They shouldn't be propagated because we marked them as "provided" anyway right? - Jarek Cecho On Nov. 19, 2015, 12:39 p.m., Dian Fu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39586/ > ----------------------------------------------------------- > > (Updated Nov. 19, 2015, 12:39 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2634 > https://issues.apache.org/jira/browse/SQOOP-2634 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > Currently Sqoop 2 has already provided the ability to config jar dependencies > with property "org.apache.sqoop.classpath.extra". The limitation of this > property is that we have to put all the dependencies together. It can't > express jar dependencies for a specified connector. This capacity is useful > as some connectors may have conflict jar dependencies. Put all the > dependencies from different connectors together may cause problems. > > > Diffs > ----- > > assemblies/pom.xml PRE-CREATION > assemblies/src/main/resources/assemblies/sqoop-connector.xml PRE-CREATION > common/src/main/java/org/apache/sqoop/error/code/ConnectorError.java > 2f17d95 > common/src/main/java/org/apache/sqoop/utils/ConnectorClassLoader.java > 4c42a78 > common/src/main/resources/org.apache.sqoop.connector-classloader.properties > e2936a9 > connector/connector-ftp/pom.xml 41ea026 > connector/connector-generic-jdbc/pom.xml 052f06d > connector/connector-hdfs/pom.xml 022a024 > connector/connector-kafka/pom.xml e0f0684 > > connector/connector-kafka/src/main/java/org/apache/sqoop/connector/kafka/KafkaToInitializer.java > 923d1aa > connector/connector-kite/pom.xml d8eaa8e > > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteFromInitializer.java > 28c5bac > > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteToInitializer.java > 50daba0 > connector/connector-oracle-jdbc/pom.xml 8186b3a > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/spi/SqoopConnector.java > 85ba8be > connector/connector-sftp/pom.xml 312ac61 > > connector/connector-sftp/src/main/java/org/apache/sqoop/connector/sftp/SftpToInitializer.java > bfb51ac > core/src/main/java/org/apache/sqoop/connector/ConnectorHandler.java 1899bb7 > core/src/main/java/org/apache/sqoop/connector/ConnectorManagerUtils.java > 9f9be57 > core/src/main/java/org/apache/sqoop/driver/JobManager.java 15ca796 > > execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java > 3acd4a1 > execution/mapreduce/src/main/java/org/apache/sqoop/job/MRJobConstants.java > 737ceda > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRUtils.java > PRE-CREATION > > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java > 0623f7b > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java > 7d20992 > > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopNullOutputFormat.java > 8c8526b > > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java > 623d1f4 > execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java > 2463643 > execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMatching.java > d0b41d1 > pom.xml 91721ce > server/pom.xml 370a6a2 > shell/src/test/java/org/apache/sqoop/shell/TestCloneCommand.java 7a139ba > shell/src/test/java/org/apache/sqoop/shell/TestCreateCommand.java 0316a60 > shell/src/test/java/org/apache/sqoop/shell/TestUpdateCommand.java d8f061a > > submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java > f396783 > > Diff: https://reviews.apache.org/r/39586/diff/ > > > Testing > ------- > > > Thanks, > > Dian Fu > >
