----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9845/#review18399 -----------------------------------------------------------
Thanks for the patch, Jarcec! I have some very minor feedback below, otherwise I think this patch is good to go. execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java <https://reviews.apache.org/r/9845/#comment38594> This can be made final. execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java <https://reviews.apache.org/r/9845/#comment38595> This can be made final. execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java <https://reviews.apache.org/r/9845/#comment38597> The general idiom is to wait for sometime after shutdown and then call shutdownNow() to ensure the executor will be shutdown eventually by interrupting the threads (the shutdown method only makes sure the threads are not scheduled again, it does not interrupt currently running threads). So you should probably change this to: progressService.shutdown(); if(!progressService.awaitTermination(5, TimeUnit.SECONDS) { progressService.shutdownNow(); } execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopReducer.java <https://reviews.apache.org/r/9845/#comment38596> This can be made final. - Hari Shreedharan On March 24, 2013, 10:56 p.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9845/ > ----------------------------------------------------------- > > (Updated March 24, 2013, 10:56 p.m.) > > > Review request for Sqoop. > > > Description > ------- > > I've ported ProgressThread from Sqoop1 and plug it into SqoopMapper and > SqoopReducer. > > > This addresses bug SQOOP-863. > https://issues.apache.org/jira/browse/SQOOP-863 > > > Diffs > ----- > > > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java > PRE-CREATION > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java > 2a823032cfd91fb8fd1a8aee3ffd9d80c2e3bae6 > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopReducer.java > d2361482771eea1a34b14c767952c5592f89c45a > > Diff: https://reviews.apache.org/r/9845/diff/ > > > Testing > ------- > > Unit and integration tests seems to be passing. I've also verified the > functionality on real cluster. > > > Thanks, > > Jarek Cecho > >
