Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Sqoop >> mail # dev >> Review Request: SQOOP-863 Sqoop2: Introduce ProgressThread into Extractor and Loader


Copy link to this message
-
Re: Review Request: SQOOP-863 Sqoop2: Introduce ProgressThread into Extractor and Loader


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

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