sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Venkat Ranganathan" <n....@live.com>
Subject Re: Review Request: Fix for SQOOP-1013
Date Mon, 10 Jun 2013 16:06:09 GMT


> On June 10, 2013, 3:36 p.m., Jarek Cecho wrote:
> > Hi Abe and Venkat,
> > thank you very much for working on this patch and supplying very valid comments!
I definitely agree that current splitters have a lot room for improvement, especially for
skewed values. However as this topic itself is very big, I would suggest to get this rather
simpler implementation in and tweak it later. What do you think?

I agree.  I will file a follow on JIRA!


> On June 10, 2013, 3:36 p.m., Jarek Cecho wrote:
> > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcImportPartitioner.java,
lines 92-93
> > <https://reviews.apache.org/r/11537/diff/2/?file=299088#file299088line92>
> >
> >     Whereas splitting on text based column is not the most optimal way how to use
Sqoop for importing data I would argue that in some situations it might be necessary. The
SQOOP-976 is more about using text based column as a base for incremental import than as a
general split by column. The main issue why we've decided not to support text based columns
for incremental import is that it's very easy to insert value "in the middle" and thus make
entire incremental import invalid. However this issue is not the case when doing normal one
time import.

I agree.  I was reviewing SQOOP-976 and I decided to disallow it as I thought it was not useful
in all situations.   But it is better to allow it for complete imports as splitting by text
is more common for complete imports.   Good catch.  Let me update the diff


> On June 10, 2013, 3:36 p.m., Jarek Cecho wrote:
> > connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestImportPartitioner.java,
line 35
> > <https://reviews.apache.org/r/11537/diff/2/?file=299089#file299089line35>
> >
> >     Extreme Nit: Would you mind removing this unused import?

Will do.  Thanks


> On June 10, 2013, 3:36 p.m., Jarek Cecho wrote:
> > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcImportPartitioner.java,
lines 415-421
> > <https://reviews.apache.org/r/11537/diff/2/?file=299088#file299088line415>
> >
> >     Nit: This method seems to be unused.

Yes.  Will remove it.  Thanks


- Venkat


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


On May 30, 2013, 6:25 p.m., Venkat Ranganathan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11537/
> -----------------------------------------------------------
> 
> (Updated May 30, 2013, 6:25 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Description
> -------
> 
> This addresses Boolean, date, time, and timestamp splitters.
> 
> THis also disallows char type splitters as discussed in SQOOP-976
> 
> 
> Diffs
> -----
> 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcImportPartitioner.java
f80f30d 
>   connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestImportPartitioner.java
ee314d0 
> 
> Diff: https://reviews.apache.org/r/11537/diff/
> 
> 
> Testing
> -------
> 
> Introduced new unit tests to test new functionality
> All tests pass
> 
> 
> Thanks,
> 
> Venkat Ranganathan
> 
>


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