sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jarek Cecho" <jar...@apache.org>
Subject Re: Review Request 28828: IDF API changes
Date Tue, 09 Dec 2014 16:13:17 GMT


> On Dec. 9, 2014, 2:48 p.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.

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


> On Dec. 9, 2014, 2:48 p.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

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.


> 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

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.


> 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

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


- Jarek


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28828/#review64374
-----------------------------------------------------------


On Dec. 9, 2014, 8: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, 8: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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message