> On March 29, 2013, 6:02 p.m., Hari Shreedharan wrote: > > 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. > > Jarek Cecho wrote: > Hi Hari, > thank you very much for your review. I appreciate your time. Thank you > very much for calling the differences: > > 1. Actually even the original patch wasn't that much configurable. It was > pretty much clean back port of Sqoop 1 class, so even though we were reading > some configuration properties, Sqoop 2 did not expose any way how to change > them. Thus it was "sort of" configurable. I've decided to change that in the > cleaner implementation. We can refactore a bit later when we properly allow > user to change the values. > 2. I don't think that having daemon thread is necessary for this purpose > to be honest. > > Jarcec
Thanks for the explanation. I agree that it need not be daemon. Thanks for the explanation regarding the configurability too. I think this patch is good to go! - Hari ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9845/#review18521 ----------------------------------------------------------- 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 > >
