> On Dec. 9, 2014, 2:48 p.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 > > Veena Basavaraj wrote: > 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 Basavaraj wrote: > to be more precise, API should never depend on its implementation, it is > always the other way round where implementation package depend on its API, at > this point, I will defer this to another ticket, but it is pretty big hackery > that way it is set up right now.
For a reference, this topic has been moved to a standalone JIRA - SQOOP-1870. > On Dec. 9, 2014, 2:48 p.m., Jarek Cecho wrote: > > execution/mapreduce/pom.xml, lines 36-39 > > <https://reviews.apache.org/r/28828/diff/3/?file=786502#file786502line36> > > > > Wondering why is execution engine depending on connector-sdk? > > Veena Basavaraj wrote: > since tests use CSV > > Jarek Cecho wrote: > I believe that mapreduce execution 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 execution engine, then perhaps the > code should not be in connector-sdk. > > Veena Basavaraj wrote: > this needs to be fixed, I am going to create a new ticket to add a > TestIDF class that can satisfy the tests, there is no need to use CSV, and > the CSV should have unit tests for itself, the integration tests should tests > it end-end Seems reasonable, thank you! - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28828/#review64374 ----------------------------------------------------------- On Dec. 9, 2014, 5:41 p.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28828/ > ----------------------------------------------------------- > > (Updated Dec. 9, 2014, 5:41 p.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-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 > > 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 > > Diff: https://reviews.apache.org/r/28828/diff/ > > > Testing > ------- > > > Thanks, > > Veena Basavaraj > >
