----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39586/#review111457 -----------------------------------------------------------
I've finished the review: connector/connector-ftp/pom.xml (line 47) <https://reviews.apache.org/r/39586/#comment171659> Nit: Incorrect ident core/src/main/java/org/apache/sqoop/core/SqoopClosure.java (line 24) <https://reviews.apache.org/r/39586/#comment171661> It seems that this interface doesn't provide much value over Java's build in Callable: https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Callable.html Wondering if we should just reuse it then? core/src/main/java/org/apache/sqoop/core/SqoopUtils.java (line 25) <https://reviews.apache.org/r/39586/#comment171660> Do you think that it would make sense to move this method to ClassUtils rather then creating new util class? https://github.com/apache/sqoop/blob/sqoop2/common/src/main/java/org/apache/sqoop/utils/ClassUtils.java execution/mapreduce/src/main/java/org/apache/sqoop/job/MRJobConstants.java (lines 69 - 70) <https://reviews.apache.org/r/39586/#comment171662> Further thinking: This property should store location of connector jar. But that location will differ on each node because server will have it somewhere else then individual mappers. It seems that the only thing we are doing with the value is to get the jar name (anything after final "/"), so I'm wondering if it would make sense to rename this to contain only connector name and use that? Or even more cleaner way would be to store from and to connector classname, then we can use the API getJarForClass to find the connectors jar name - this would be more in line with how we're shipping the rest of the classes. What do you think? submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java (lines 203 - 205) <https://reviews.apache.org/r/39586/#comment171663> I'm wondering why we are using the tmpfiles rather then tmpjars? Having the connector jars on the classpath shouldn't affect anything right? I mean, even the server have the connector jars on it's classpath - we're just using different classloader to load classes from those jars. - Jarek Cecho On Dec. 21, 2015, 6:23 a.m., Dian Fu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39586/ > ----------------------------------------------------------- > > (Updated Dec. 21, 2015, 6:23 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2634 > https://issues.apache.org/jira/browse/SQOOP-2634 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > The aim of this JIRA is to provide classpath isolation for connectors and its > dependencies. It's achieved in the following method: > 1. Package connector jar with its dependencies. There will be a directory lib > which contains the dependencies of the connector > 2. Improve the ConnectorClassLoader to make it can load classes from the > dependencies which are inside the connector jar (SQOOP-2635) > 3. Load connector class with the connector's own ConnectorClassLoader. > > > Diffs > ----- > > assemblies/pom.xml PRE-CREATION > assemblies/src/main/resources/assemblies/sqoop-connector.xml PRE-CREATION > common/src/main/java/org/apache/sqoop/classloader/ConnectorClassLoader.java > 370de2a > common/src/main/java/org/apache/sqoop/error/code/ConnectorError.java > 2f17d95 > > common/src/test/java/org/apache/sqoop/classloader/TestConnectorClassLoader.java > 87edf3d > common/src/test/java/org/apache/sqoop/utils/TestClassUtils.java ec48f82 > 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/ConnectorManager.java f19f391 > core/src/main/java/org/apache/sqoop/connector/ConnectorManagerUtils.java > 9f9be57 > core/src/main/java/org/apache/sqoop/core/SqoopClosure.java PRE-CREATION > core/src/main/java/org/apache/sqoop/core/SqoopUtils.java PRE-CREATION > core/src/main/java/org/apache/sqoop/driver/JobManager.java d3a750e > > core/src/test/java/org/apache/sqoop/connector/TestConnectorManagerUtils.java > 423b3df > > 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 > > execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestSqoopOutputFormatLoadExecutor.java > 3dee8f6 > pom.xml 460273a > server/pom.xml 370a6a2 > > 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 > >
