-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9845/
-----------------------------------------------------------

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9845/#review17681
-----------------------------------------------------------
Generally looks good, but it'd be better to use the more modern thread scheduling capabilities in Java.
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressThread.java
<https://reviews.apache.org/r/9845/#comment37551>

    Why don't you simply use a ScheduledSingleThreadedExecutor to do this? You can get rid of all of the time related stuff and the infinite loop. In addition to making the code cleaner, we can avoid one additional thread
- Hari Shreedharan
On March 11, 2013, 2:53 a.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9845/
> -----------------------------------------------------------
>
> (Updated March 11, 2013, 2:53 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/ProgressThread.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
>
>

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

I've reworked the patch based on Hari's suggestion.
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 (updated)
-----

  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

> On March 11, 2013, 6:13 a.m., Hari Shreedharan wrote:
> > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressThread.java, lines 106-146
> > <https://reviews.apache.org/r/9845/diff/1/?file=268966#file268966line106>
> >
> >     Why don't you simply use a ScheduledSingleThreadedExecutor to do this? You can get rid of all of the time related stuff and the infinite loop. In addition to making the code cleaner, we can avoid one additional thread

Thank you for your suggestion Hari!
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9845/#review17681
-----------------------------------------------------------
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
>
>

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

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

I've applied Hari's suggestions.
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 (updated)
-----

  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

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

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9845/#review18564
-----------------------------------------------------------

Ship it!
Ship It!

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

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9845/#review18758
-----------------------------------------------------------

Ship it!
Ship It!

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