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: SQOOP-390: PostgreSQL connector for direct export with pg_bulkload
Date Thu, 26 Jul 2012 21:13:37 GMT

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


Hi Masatake,
I've read more carefully this time through your code and I do have few comments. I didn't
have time to transfer some bits yet, so this is definitely not my final review. Please feel
free to wait with your actions until I'll be done. I mainly wanted to provide you feedback
and show you that I'm actually working on it this time :-)


/src/java/org/apache/sqoop/mapreduce/AutoProgressReducer.java
<https://reviews.apache.org/r/2724/#comment20335>

    This class seems to be more Reducer than Mapper :-) Would you mind fixing the javadoc?



/src/java/org/apache/sqoop/mapreduce/AutoProgressReducer.java
<https://reviews.apache.org/r/2724/#comment20342>

    I've notice that this class is to very hight extent identical to Sqoop's AutoProgressMapper.
I'm thinking that rather then keeping similar code base twice, it might be beneficial to abstract
shared functionality to separate class and simply reuse it in both AutoProgress[Mapper|Reducer].
What do you think?



/src/java/org/apache/sqoop/mapreduce/AutoProgressReducer.java
<https://reviews.apache.org/r/2724/#comment20336>

    Could you add stack trace printing here -- I believe it might be useful. Something like:
    
    Log.warn("..." + ie.toString(), ie);



/src/java/org/apache/sqoop/mapreduce/PGBulkloadExportJob.java
<https://reviews.apache.org/r/2724/#comment20337>

    You're also setting number of reducers on line 169 of this file. I believe that it's not
necessary to do it twice.



/src/java/org/apache/sqoop/mapreduce/PGBulkloadExportJob.java
<https://reviews.apache.org/r/2724/#comment20338>

    This exactly same handling of two different exceptions seems weird to me. What about putting
only one catch class that will catch Exception (i.e everything)?



/src/java/org/apache/sqoop/mapreduce/PGBulkloadExportJob.java
<https://reviews.apache.org/r/2724/#comment20343>

    I personally do not like this table searching. We might delete some tables that user left
there intentionally from previous export. I would more prefer to simply not support --clear-staging-table
parameter (properly die if it will be specified and document this feature in the user guide).
What do you think?



/src/java/org/apache/sqoop/mapreduce/PGBulkloadExportReducer.java
<https://reviews.apache.org/r/2724/#comment20340>

    I would suggest putting here just one catch class for Exception and effectively catch
up everything.



/src/java/org/apache/sqoop/mapreduce/PGBulkloadExportReducer.java
<https://reviews.apache.org/r/2724/#comment20341>

    I'm thinking whether it would be beneficial here not to die immediately. For example if
user is exporting 30 partitions and there will be some issues with the first. He would be
forced to manually do all 30 of them. But if we would try all them first and log only the
failed ones, user would have to fix only that one. What do you think sir?


I also noticed that you've done very good job documenting usage on the JIRA. There is a lot
of extra arguments that user needs to set up, so I would prefer if you could also upgrade
sqoop user guide in this patch.

Jarcec

- Jarek Cecho


On July 26, 2012, 10:41 a.m., Masatake Iwasaki wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2724/
> -----------------------------------------------------------
> 
> (Updated July 26, 2012, 10:41 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Description
> -------
> 
> Patch for SQOOP-390
> https://issues.apache.org/jira/browse/SQOOP-390
> 
> 
> This addresses bug SQOOP-390.
>     https://issues.apache.org/jira/browse/SQOOP-390
> 
> 
> Diffs
> -----
> 
>   /src/java/org/apache/sqoop/manager/PGBulkloadManager.java PRE-CREATION 
>   /src/java/org/apache/sqoop/mapreduce/AutoProgressReducer.java PRE-CREATION 
>   /src/java/org/apache/sqoop/mapreduce/PGBulkloadExportJob.java PRE-CREATION 
>   /src/java/org/apache/sqoop/mapreduce/PGBulkloadExportMapper.java PRE-CREATION 
>   /src/java/org/apache/sqoop/mapreduce/PGBulkloadExportReducer.java PRE-CREATION 
>   /src/test/com/cloudera/sqoop/manager/PGBulkloadManagerManualTest.java PRE-CREATION

> 
> Diff: https://reviews.apache.org/r/2724/diff/
> 
> 
> Testing
> -------
> 
> This patch include the test class PGBulkloadManagerTest.
> I've tested "ant test" and passed.
> 
> 
> Thanks,
> 
> Masatake Iwasaki
> 
>


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