Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Sqoop, mail # dev - Review Request 12936: SQOOP-777. Sqoop2: Pluggable Intermediate Data Format


Copy link to this message
-
Re: Review Request 12936: SQOOP-777. Sqoop2: Pluggable Intermediate Data Format
Hari Shreedharan 2013-08-01, 03:40


> On July 31, 2013, 11:25 p.m., Jarek Cecho wrote:
> > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java, line 78
> > <https://reviews.apache.org/r/12936/diff/4/?file=329175#file329175line78>
> >
> >     Can we throw more meaning full exception here? :-)

I added this to figure out which tests were failing because of lack of schema. This check is not required, since the schema would exist in the conf anyway.
> On July 31, 2013, 11:25 p.m., Jarek Cecho wrote:
> > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportInitializer.java, lines 71-74
> > <https://reviews.apache.org/r/12936/diff/4/?file=329160#file329160line71>
> >
> >     I'm afraid that the schema generation for query based export will fail and I'm not sure how to implement that, so I we might need to remove that functionality.

For now, I am going to leave it around. We should work on removing that in a separate jira.
> On July 31, 2013, 11:25 p.m., Jarek Cecho wrote:
> > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportInitializer.java, lines 80-100
> > <https://reviews.apache.org/r/12936/diff/4/?file=329160#file329160line80>
> >
> >     Similar code is also in importer, so maybe we could refactore that into some Util/Helper class?

Done.
> On July 31, 2013, 11:25 p.m., Jarek Cecho wrote:
> > core/src/main/java/org/apache/sqoop/framework/FrameworkConstants.java, lines 59-61
> > <https://reviews.apache.org/r/12936/diff/4/?file=329163#file329163line59>
> >
> >     Nit: This property seems to be used mainly within the Execute/Submission engine, so it's better placed in JobConstants instead.

Done
> On July 31, 2013, 11:25 p.m., Jarek Cecho wrote:
> > pom.xml, lines 326-330
> > <https://reviews.apache.org/r/12936/diff/4/?file=329183#file329183line326>
> >
> >     We already have guava dependency in Hadoop 100 profile. If the goal here is to make it explicit, I would suggest to move it out from there and use the property guava.version to specify the version.

Done
> On July 31, 2013, 11:25 p.m., Jarek Cecho wrote:
> > common/src/main/java/org/apache/sqoop/etl/io/CSVIntermediateDataFormat.java, line 19
> > <https://reviews.apache.org/r/12936/diff/4/?file=329153#file329153line19>
> >
> >     I would much rather see such class(es) in connector sdk than in common module (that is for example shared with client).

Done
> On July 31, 2013, 11:25 p.m., Jarek Cecho wrote:
> > common/src/main/java/org/apache/sqoop/etl/io/DataReader.java, lines 24-34
> > <https://reviews.apache.org/r/12936/diff/4/?file=329154#file329154line24>
> >
> >     Nit: Can we add java docs describing purpose of each method?

Done
> On July 31, 2013, 11:25 p.m., Jarek Cecho wrote:
> > common/src/main/java/org/apache/sqoop/etl/io/DataWriter.java, lines 24-34
> > <https://reviews.apache.org/r/12936/diff/4/?file=329155#file329155line24>
> >
> >     Nit: Can we add java docs describing purpose of each method?

DOne
> On July 31, 2013, 11:25 p.m., Jarek Cecho wrote:
> > common/src/main/java/org/apache/sqoop/etl/io/IntermediateDataFormat.java, lines 119-125
> > <https://reviews.apache.org/r/12936/diff/4/?file=329156#file329156line119>
> >
> >     Nit: Please add the java docs here as well.

Done
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12936/#review24352
-----------------------------------------------------------
On July 27, 2013, 4:11 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12936/
> -----------------------------------------------------------
>
> (Updated July 27, 2013, 4:11 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-777
>     https://issues.apache.org/jira/browse/SQOOP-777
>
>
> Repository: sqoop-sqoop2