----------------------------------------------------------- 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 > >
