> On Dec. 9, 2014, 6:48 a.m., Jarek Cecho wrote: > > core/pom.xml, lines 42-44 > > <https://reviews.apache.org/r/28828/diff/3/?file=786499#file786499line42> > > > > Wondering why is core depending on joda-time directly? > > Veena Basavaraj wrote: > jobManaager. > > Jarek Cecho wrote: > I see, the JobManager is explicitly naming some classes from JODA to be > included in the job [1]. I think that the change is fine for now, but we > should change it in subsequent JIRA. This way the core would have to depend > on everything and the idea of having lightweight core would be gone. We > should provide a way how the IDF will expose dependent jars and add them into > the class path similar way as we are doing for connectors. Or perhaps it > should be the connector who will report the dependent jars as it's connector > who is picking the IDF. > > Links: > 1: > https://github.com/apache/sqoop/blob/sqoop2/core/src/main/java/org/apache/sqoop/driver/JobManager.java#L402
fair point. > On Dec. 9, 2014, 6:48 a.m., Jarek Cecho wrote: > > core/pom.xml, lines 57-59 > > <https://reviews.apache.org/r/28828/diff/3/?file=786499#file786499line57> > > > > I'm wondering why would core depend on connector-sdk? > > Veena Basavaraj wrote: > since there are tests that use CSV > > Jarek Cecho wrote: > I believe that core should not depend on connector-sdk from the same > reason why it's not depending on any other connectors. If we have > functionality that is required in core, then perhaps the code should not be > in connector-sdk. fixed. my bad, it doesnot need it > On Dec. 9, 2014, 6:48 a.m., Jarek Cecho wrote: > > spi/src/main/java/org/apache/sqoop/connector/spi/SqoopConnector.java, line > > 95 > > <https://reviews.apache.org/r/28828/diff/3/?file=786511#file786511line95> > > > > I'm wondering why this change? It seems reasonable to have default > > value. > > Veena Basavaraj wrote: > if we have that then we need to think of how SPI does not deepend on SDK, > and it creates a cyclic dependency > > Jarek Cecho wrote: > I see, that is a good point. I think that the cyclic dependency is the > reason why Hari put the IDF implementation into the connector-sdk as well in > the first place (as we've discussed in [1]). Let's perhaps not change that at > this point as it obviously requires more thinking as we are quite heavily > changing the dependencies and not just the API as we originally intented to? > > Links: > 1: > https://issues.apache.org/jira/browse/SQOOP-1811?focusedCommentId=14236216&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14236216 IMO it was a hack Hari did , and it is not right. and IDF API is not part of SDK, then we should add everything in SPI. the only dependency we have is tests using CSV, the tests should use a TEST CSV or mock it and not rely on a actual class, this is the way it should have been, - Veena ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28828/#review64374 ----------------------------------------------------------- On Dec. 9, 2014, 12:17 a.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28828/ > ----------------------------------------------------------- > > (Updated Dec. 9, 2014, 12:17 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1811 > https://issues.apache.org/jira/browse/SQOOP-1811 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > see jira > > > Diffs > ----- > > connector/connector-generic-jdbc/pom.xml fc6cab4 > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnector.java > c233ed5 > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnector.java > f70142d > connector/connector-kite/pom.xml 10ed099 > > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConnector.java > c864882 > connector/connector-sdk/pom.xml 38c217a > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java > d6470e6 > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatError.java > PRE-CREATION > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java > 253dfba > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormatError.java > 4b0dd88 > > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java > 3e7c0d1 > core/pom.xml 2b6e436 > core/src/main/java/org/apache/sqoop/driver/JobManager.java f4f5561 > core/src/main/java/org/apache/sqoop/driver/JobRequest.java eed79a5 > execution/mapreduce/pom.xml b23b905 > > execution/mapreduce/src/main/java/org/apache/sqoop/job/io/SqoopWritable.java > 05b731a > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java > b9dd11d > > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java > 49a66b9 > execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java > bbac7d2 > execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMatching.java > a64a4a6 > > execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestSqoopOutputFormatLoadExecutor.java > 7c40ad5 > > execution/mapreduce/src/test/java/org/apache/sqoop/job/util/MRJobTestUtil.java > 5d5359e > spi/pom.xml 43f17d4 > spi/src/main/java/org/apache/sqoop/connector/spi/SqoopConnector.java > ff6392e > spi/src/main/java/org/apache/sqoop/idf/IntermediateDataFormat.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/28828/diff/ > > > Testing > ------- > > > Thanks, > > Veena Basavaraj > >
