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

Review request for Sqoop and Hari Shreedharan.
Description
-------

I've changed unconditional free.acquire() to do-while loop that will periodically check reader status.
This addresses bug SQOOP-674.
    https://issues.apache.org/jira/browse/SQOOP-674
Diffs
-----

  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java 38e2292781b10bab577f3aa8af47d2d0c9365e46

Diff: https://reviews.apache.org/r/8622/diff/
Testing
-------

Unit tests are passing and I've tested it on real cluster.
Thanks,

Jarek Cecho

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

Excellent catch! Running tryAcquire in a loop is generally an expensive CPU consuming operation. You could do something like this:

* Release the free semaphore after setting readerFinished to true, just before throwing the exception in the consumer whenever exceptions are thrown (2 places I think).
* Exchange the order of calls to free.acquire and checkIfConsumerThrew (acquire the free sema before checking if the consumer threw).

This would make sure that we don't try to acquire the semaphore in a loop, but yet ensure that we will release the semaphore when an exception is thrown so we can continue and throw the exception out

- Hari Shreedharan
On Dec. 16, 2012, 3:22 a.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8622/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2012, 3:22 a.m.)
>
>
> Review request for Sqoop and Hari Shreedharan.
>
>
> Description
> -------
>
> I've changed unconditional free.acquire() to do-while loop that will periodically check reader status.
>
>
> This addresses bug SQOOP-674.
>     https://issues.apache.org/jira/browse/SQOOP-674
>
>
> Diffs
> -----
>
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java 38e2292781b10bab577f3aa8af47d2d0c9365e46
>
> Diff: https://reviews.apache.org/r/8622/diff/
>
>
> Testing
> -------
>
> Unit tests are passing and I've tested it on real cluster.
>
>
> Thanks,
>
> Jarek Cecho
>
>

> On Dec. 16, 2012, 10:12 p.m., Hari Shreedharan wrote:
> > Jarcec,
> >
> > Excellent catch! Running tryAcquire in a loop is generally an expensive CPU consuming operation. You could do something like this:
> >
> > * Release the free semaphore after setting readerFinished to true, just before throwing the exception in the consumer whenever exceptions are thrown (2 places I think).
> > * Exchange the order of calls to free.acquire and checkIfConsumerThrew (acquire the free sema before checking if the consumer threw).
> >
> > This would make sure that we don't try to acquire the semaphore in a loop, but yet ensure that we will release the semaphore when an exception is thrown so we can continue and throw the exception out

Also I'd recommend adding a unit test. It should be fairly easy to write a dummy loader that throws after a fixed number of operations. Such a test would fail without this patch.
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8622/#review14569
-----------------------------------------------------------
On Dec. 16, 2012, 3:22 a.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8622/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2012, 3:22 a.m.)
>
>
> Review request for Sqoop and Hari Shreedharan.
>
>
> Description
> -------
>
> I've changed unconditional free.acquire() to do-while loop that will periodically check reader status.
>
>
> This addresses bug SQOOP-674.
>     https://issues.apache.org/jira/browse/SQOOP-674
>
>
> Diffs
> -----
>
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java 38e2292781b10bab577f3aa8af47d2d0c9365e46
>
> Diff: https://reviews.apache.org/r/8622/diff/
>
>
> Testing
> -------
>
> Unit tests are passing and I've tested it on real cluster.
>
>
> Thanks,
>
> Jarek Cecho
>
>

> On Dec. 16, 2012, 10:12 p.m., Hari Shreedharan wrote:
> > Jarcec,
> >
> > Excellent catch! Running tryAcquire in a loop is generally an expensive CPU consuming operation. You could do something like this:
> >
> > * Release the free semaphore after setting readerFinished to true, just before throwing the exception in the consumer whenever exceptions are thrown (2 places I think).
> > * Exchange the order of calls to free.acquire and checkIfConsumerThrew (acquire the free sema before checking if the consumer threw).
> >
> > This would make sure that we don't try to acquire the semaphore in a loop, but yet ensure that we will release the semaphore when an exception is thrown so we can continue and throw the exception out
>
> Hari Shreedharan wrote:
>     Also I'd recommend adding a unit test. It should be fairly easy to write a dummy loader that throws after a fixed number of operations. Such a test would fail without this patch.

Hi Hari,
thank you for your review and suggestions, I appreciate your time.

I do not quite agree that tryAcquire is in this case expensive operation as proposed loop is waiting 5 seconds and frankly it's extremely unlikely that it will be executed more than once. Because if it would be executed more than once, than it means that we need more then 5 seconds to process one single entry and that's much bigger problem than small 5s loop.

I like your suggestion more than mine as it's more cleaner, so please, do not hesitate and jump in with cleaner solution! :-)

Jarcec
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8622/#review14569
-----------------------------------------------------------
On Dec. 16, 2012, 3:22 a.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8622/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2012, 3:22 a.m.)
>
>
> Review request for Sqoop and Hari Shreedharan.
>
>
> Description
> -------
>
> I've changed unconditional free.acquire() to do-while loop that will periodically check reader status.
>
>
> This addresses bug SQOOP-674.
>     https://issues.apache.org/jira/browse/SQOOP-674
>
>
> Diffs
> -----
>
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java 38e2292781b10bab577f3aa8af47d2d0c9365e46
>
> Diff: https://reviews.apache.org/r/8622/diff/
>
>
> Testing
> -------
>
> Unit tests are passing and I've tested it on real cluster.
>
>
> Thanks,
>
> Jarek Cecho
>
>

> On Dec. 16, 2012, 10:12 p.m., Hari Shreedharan wrote:
> > Jarcec,
> >
> > Excellent catch! Running tryAcquire in a loop is generally an expensive CPU consuming operation. You could do something like this:
> >
> > * Release the free semaphore after setting readerFinished to true, just before throwing the exception in the consumer whenever exceptions are thrown (2 places I think).
> > * Exchange the order of calls to free.acquire and checkIfConsumerThrew (acquire the free sema before checking if the consumer threw).
> >
> > This would make sure that we don't try to acquire the semaphore in a loop, but yet ensure that we will release the semaphore when an exception is thrown so we can continue and throw the exception out
>
> Hari Shreedharan wrote:
>     Also I'd recommend adding a unit test. It should be fairly easy to write a dummy loader that throws after a fixed number of operations. Such a test would fail without this patch.
>
> Jarek Cecho wrote:
>     Hi Hari,
>     thank you for your review and suggestions, I appreciate your time.
>    
>     I do not quite agree that tryAcquire is in this case expensive operation as proposed loop is waiting 5 seconds and frankly it's extremely unlikely that it will be executed more than once. Because if it would be executed more than once, than it means that we need more then 5 seconds to process one single entry and that's much bigger problem than small 5s loop.
>    
>     I like your suggestion more than mine as it's more cleaner, so please, do not hesitate and jump in with cleaner solution! :-)
>    
>     Jarcec

Jarcec,

If there is code that is sitting in a loop and trying to acquire a semaphore - that just means you have deeper threading problems. If you want to actually wait for 5 seconds every time we cannot acquire, why use semaphores at all? Just keep state variables and add a sleep call that sleeps for 5 seconds. The whole reason we use semaphores is for a cleaner solution. Also agreed that CPU intensive is actually relative. Anything that sits in a loop and waits is basically CPU intensive in one way or the other.
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8622/#review14569
-----------------------------------------------------------
On Dec. 16, 2012, 3:22 a.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8622/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2012, 3:22 a.m.)
>
>
> Review request for Sqoop and Hari Shreedharan.
>
>
> Description
> -------
>
> I've changed unconditional free.acquire() to do-while loop that will periodically check reader status.
>
>
> This addresses bug SQOOP-674.
>     https://issues.apache.org/jira/browse/SQOOP-674
>
>
> Diffs
> -----
>
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java 38e2292781b10bab577f3aa8af47d2d0c9365e46
>
> Diff: https://reviews.apache.org/r/8622/diff/
>
>
> Testing
> -------
>
> Unit tests are passing and I've tested it on real cluster.
>
>
> Thanks,
>
> Jarek Cecho
>
>

> On Dec. 16, 2012, 10:12 p.m., Hari Shreedharan wrote:
> > Jarcec,
> >
> > Excellent catch! Running tryAcquire in a loop is generally an expensive CPU consuming operation. You could do something like this:
> >
> > * Release the free semaphore after setting readerFinished to true, just before throwing the exception in the consumer whenever exceptions are thrown (2 places I think).
> > * Exchange the order of calls to free.acquire and checkIfConsumerThrew (acquire the free sema before checking if the consumer threw).
> >
> > This would make sure that we don't try to acquire the semaphore in a loop, but yet ensure that we will release the semaphore when an exception is thrown so we can continue and throw the exception out
>
> Hari Shreedharan wrote:
>     Also I'd recommend adding a unit test. It should be fairly easy to write a dummy loader that throws after a fixed number of operations. Such a test would fail without this patch.
>
> Jarek Cecho wrote:
>     Hi Hari,
>     thank you for your review and suggestions, I appreciate your time.
>    
>     I do not quite agree that tryAcquire is in this case expensive operation as proposed loop is waiting 5 seconds and frankly it's extremely unlikely that it will be executed more than once. Because if it would be executed more than once, than it means that we need more then 5 seconds to process one single entry and that's much bigger problem than small 5s loop.
>    
>     I like your suggestion more than mine as it's more cleaner, so please, do not hesitate and jump in with cleaner solution! :-)
>    
>     Jarcec
>
> Hari Shreedharan wrote:
>     Jarcec,
>    
>     If there is code that is sitting in a loop and trying to acquire a semaphore - that just means you have deeper threading problems. If you want to actually wait for 5 seconds every time we cannot acquire, why use semaphores at all? Just keep state variables and add a sleep call that sleeps for 5 seconds. The whole reason we use semaphores is for a cleaner solution. Also agreed that CPU intensive is actually relative. Anything that sits in a loop and waits is basically CPU intensive in one way or the other.

Jarcec - I have some time today - if you want me to review a new patch or if you would like me to provide a patch - if you don't want to work on this.
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8622/#review14569
-----------------------------------------------------------
On Dec. 16, 2012, 3:22 a.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8622/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2012, 3:22 a.m.)
>
>
> Review request for Sqoop and Hari Shreedharan.
>
>
> Description
> -------
>
> I've changed unconditional free.acquire() to do-while loop that will periodically check reader status.
>
>
> This addresses bug SQOOP-674.
>     https://issues.apache.org/jira/browse/SQOOP-674
>
>
> Diffs
> -----
>
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java 38e2292781b10bab577f3aa8af47d2d0c9365e46
>
> Diff: https://reviews.apache.org/r/8622/diff/
>
>
> Testing
> -------
>
> Unit tests are passing and I've tested it on real cluster.
>
>
> Thanks,
>
> Jarek Cecho
>
>

> On Dec. 16, 2012, 10:12 p.m., Hari Shreedharan wrote:
> > Jarcec,
> >
> > Excellent catch! Running tryAcquire in a loop is generally an expensive CPU consuming operation. You could do something like this:
> >
> > * Release the free semaphore after setting readerFinished to true, just before throwing the exception in the consumer whenever exceptions are thrown (2 places I think).
> > * Exchange the order of calls to free.acquire and checkIfConsumerThrew (acquire the free sema before checking if the consumer threw).
> >
> > This would make sure that we don't try to acquire the semaphore in a loop, but yet ensure that we will release the semaphore when an exception is thrown so we can continue and throw the exception out
>
> Hari Shreedharan wrote:
>     Also I'd recommend adding a unit test. It should be fairly easy to write a dummy loader that throws after a fixed number of operations. Such a test would fail without this patch.
>
> Jarek Cecho wrote:
>     Hi Hari,
>     thank you for your review and suggestions, I appreciate your time.
>    
>     I do not quite agree that tryAcquire is in this case expensive operation as proposed loop is waiting 5 seconds and frankly it's extremely unlikely that it will be executed more than once. Because if it would be executed more than once, than it means that we need more then 5 seconds to process one single entry and that's much bigger problem than small 5s loop.
>    
>     I like your suggestion more than mine as it's more cleaner, so please, do not hesitate and jump in with cleaner solution! :-)
>    
>     Jarcec
>
> Hari Shreedharan wrote:
>     Jarcec,
>    
>     If there is code that is sitting in a loop and trying to acquire a semaphore - that just means you have deeper threading problems. If you want to actually wait for 5 seconds every time we cannot acquire, why use semaphores at all? Just keep state variables and add a sleep call that sleeps for 5 seconds. The whole reason we use semaphores is for a cleaner solution. Also agreed that CPU intensive is actually relative. Anything that sits in a loop and waits is basically CPU intensive in one way or the other.
>
> Hari Shreedharan wrote:
>     Jarcec - I have some time today - if you want me to review a new patch or if you would like me to provide a patch - if you don't want to work on this.

Hi Hari,
if you have time, please go ahead and submit new patch! I'll be more than happy to review it and commit it. I liked your solution more anyway :-)

Jarcec
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8622/#review14569
-----------------------------------------------------------
On Dec. 16, 2012, 3:22 a.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8622/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2012, 3:22 a.m.)
>
>
> Review request for Sqoop and Hari Shreedharan.
>
>
> Description
> -------
>
> I've changed unconditional free.acquire() to do-while loop that will periodically check reader status.
>
>
> This addresses bug SQOOP-674.
>     https://issues.apache.org/jira/browse/SQOOP-674
>
>
> Diffs
> -----
>
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java 38e2292781b10bab577f3aa8af47d2d0c9365e46
>
> Diff: https://reviews.apache.org/r/8622/diff/
>
>
> Testing
> -------
>
> Unit tests are passing and I've tested it on real cluster.
>
>
> Thanks,
>
> Jarek Cecho
>
>