----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9845/#review18521 -----------------------------------------------------------
Hi Jarcec, This patch looks great. This is committable as-is. The only question I have is that the behavior of the original patch (rev 1) and this one have changed in the following ways: 1. The time between the progress calls was configurable in the first patch, but not this one (came from the context instance I guess - in the first patch) 2. The thread was daemon in rev 1, but it is no longer (you can pass in a thread factory to the newScheduledSingleThreadedExecutor call and make your thread daemon before returning - you can also use Guava's ThreadFactoryBuilder if you want). By passing in a thread factory or using ThreadFactoryBuilder, you can name the thread too. If these two are not much of a concern, +1 from me. - Hari Shreedharan On March 27, 2013, 12:40 a.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9845/ > ----------------------------------------------------------- > > (Updated March 27, 2013, 12:40 a.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 > >
