> On Nov. 15, 2012, 1:53 a.m., Juhani Connolly wrote:
> > Hi Hari, you just posted a sqoop issue to the flume group to review? Was this your intention... I had a glance at the original issue but it didn't look related to flume?
>
> Hari Shreedharan wrote:
>     Sorry, old habits die hard! ;)
>    
>     It was not intended for Flume, but for Sqoop!
>    
>     Sorry for the spam!

No problem, I guessed that might be the case, and just informed you so the right people could see this :)
- Juhani
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8063/#review13456
-----------------------------------------------------------
On Nov. 15, 2012, 1:59 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8063/
> -----------------------------------------------------------
>
> (Updated Nov. 15, 2012, 1:59 a.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Improved thread handling in SqoopOutputFormatLoadExecutor. Removed the synchronized blocks and wait/notify.
>
>
> This addresses bug SQOOP-690.
>     https://issues.apache.org/jira/browse/SQOOP-690
>
>
> Diffs
> -----
>
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/io/Data.java f6fff0b
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java 0d636ae
>
> Diff: https://reviews.apache.org/r/8063/diff/
>
>
> Testing
> -------
>
> Ran unit tests, and on a real cluster.
>
>
> Thanks,
>
> Hari Shreedharan
>
>

I've actually seen it without any issues :-)

Jarcec

On Fri, Nov 16, 2012 at 02:03:18AM -0000, Juhani Connolly wrote:
>
>
> > On Nov. 15, 2012, 1:53 a.m., Juhani Connolly wrote:
> > > Hi Hari, you just posted a sqoop issue to the flume group to review? Was this your intention... I had a glance at the original issue but it didn't look related to flume?
> >
> > Hari Shreedharan wrote:
> >     Sorry, old habits die hard! ;)
> >    
> >     It was not intended for Flume, but for Sqoop!
> >    
> >     Sorry for the spam!
>
> No problem, I guessed that might be the case, and just informed you so the right people could see this :)
>
>
> - Juhani
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8063/#review13456
> -----------------------------------------------------------
>
>
> On Nov. 15, 2012, 1:59 a.m., Hari Shreedharan wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/8063/
> > -----------------------------------------------------------
> >
> > (Updated Nov. 15, 2012, 1:59 a.m.)
> >
> >
> > Review request for Sqoop.
> >
> >
> > Description
> > -------
> >
> > Improved thread handling in SqoopOutputFormatLoadExecutor. Removed the synchronized blocks and wait/notify.
> >
> >
> > This addresses bug SQOOP-690.
> >     https://issues.apache.org/jira/browse/SQOOP-690
> >
> >
> > Diffs
> > -----
> >
> >   execution/mapreduce/src/main/java/org/apache/sqoop/job/io/Data.java f6fff0b
> >   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java 0d636ae
> >
> > Diff: https://reviews.apache.org/r/8063/diff/
> >
> >
> > Testing
> > -------
> >
> > Ran unit tests, and on a real cluster.
> >
> >
> > Thanks,
> >
> > Hari Shreedharan
> >
> >
>

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

(Updated Nov. 15, 2012, 1:59 a.m.)
Review request for Sqoop.
Description
-------

Improved thread handling in SqoopOutputFormatLoadExecutor. Removed the synchronized blocks and wait/notify.
This addresses bug SQOOP-690.
    https://issues.apache.org/jira/browse/SQOOP-690
Diffs
-----

  execution/mapreduce/src/main/java/org/apache/sqoop/job/io/Data.java f6fff0b
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java 0d636ae

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

Ran unit tests, and on a real cluster.
Thanks,

Hari Shreedharan

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

(Updated Nov. 16, 2012, 9:48 a.m.)
Review request for Sqoop.
Changes
-------

Rewrote the patch using semaphores, because I hit some weird reference return issues with the previous patch.
Description
-------

Improved thread handling in SqoopOutputFormatLoadExecutor. Removed the synchronized blocks and wait/notify.
This addresses bug SQOOP-690.
    https://issues.apache.org/jira/browse/SQOOP-690
Diffs (updated)
-----

  execution/mapreduce/src/main/java/org/apache/sqoop/job/io/Data.java 41fceb8
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java 0d636ae

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

Ran unit tests, and on a real cluster.
Thanks,

Hari Shreedharan

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8063/#review13520
-----------------------------------------------------------
Hi Hari,
thank you very much for your patch. Couple of nits:

1) Would you mind putting space between "//" in your comments? E.g. "// This is comment" rather than "//This is comment"
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java
<https://reviews.apache.org/r/8063/#comment28983>

    I'm not sure if this comment is still valid.

execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java
<https://reviews.apache.org/r/8063/#comment28984>

    I believe that the comment is no longer valid right as we're not using "yield".

execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java
<https://reviews.apache.org/r/8063/#comment28986>

    Typo in world "inerrupt".

execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java
<https://reviews.apache.org/r/8063/#comment28987>

    Rhetorical question. Can it happen that filled.acquire will throw InterruptedException exception when writerFinished = false? Because if so we might consider putting this code into loop or call acquireUninterruptibly instead.

execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java
<https://reviews.apache.org/r/8063/#comment28985>

    I would suggest to log the exception in addition to storing it for later usage.
Jarcec

- Jarek Cecho
On Nov. 16, 2012, 9:48 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8063/
> -----------------------------------------------------------
>
> (Updated Nov. 16, 2012, 9:48 a.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Improved thread handling in SqoopOutputFormatLoadExecutor. Removed the synchronized blocks and wait/notify.
>
>
> This addresses bug SQOOP-690.
>     https://issues.apache.org/jira/browse/SQOOP-690
>
>
> Diffs
> -----
>
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/io/Data.java 41fceb8
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java 0d636ae
>
> Diff: https://reviews.apache.org/r/8063/diff/
>
>
> Testing
> -------
>
> Ran unit tests, and on a real cluster.
>
>
> Thanks,
>
> Hari Shreedharan
>
>

> On Nov. 16, 2012, 5:22 p.m., Jarek Cecho wrote:
> > Hi Hari,
> > thank you very much for your patch. Couple of nits:
> >
> > 1) Would you mind putting space between "//" in your comments? E.g. "// This is comment" rather than "//This is comment"

Didn't know there was this guideline. Will do.
> On Nov. 16, 2012, 5:22 p.m., Jarek Cecho wrote:
> > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java, lines 160-165
> > <https://reviews.apache.org/r/8063/diff/3/?file=190639#file190639line160>
> >
> >     Rhetorical question. Can it happen that filled.acquire will throw InterruptedException exception when writerFinished = false? Because if so we might consider putting this code into loop or call acquireUninterruptibly instead.

I don't like to use acquireUninterruptibly. The JVM will interrupt (and should) this thread if needed, and that should be handled (this will become more important when I make consumer a non-daemon thread). Sitting in a loop and checking will cause the acquire call to be made several times (in an earlier version I was using acquire(timeout), and then checking for writerFinished, but I like this better. That said, I should re-throw the interrupted exception so that it is stashed for later use.

I used acquireUninteeruptibly in case of the read methods for testing, forgot to change them to acquire.

To throw the interrupted exception I changed the loader interface to allow it to throw checked exceptions (why was this not done earlier?)
> On Nov. 16, 2012, 5:22 p.m., Jarek Cecho wrote:
> > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java, line 215
> > <https://reviews.apache.org/r/8063/diff/3/?file=190639#file190639line215>
> >
> >     I would suggest to log the exception in addition to storing it for later usage.

Agreed.
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8063/#review13520
-----------------------------------------------------------
On Nov. 16, 2012, 9:48 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8063/
> -----------------------------------------------------------
>
> (Updated Nov. 16, 2012, 9:48 a.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Improved thread handling in SqoopOutputFormatLoadExecutor. Removed the synchronized blocks and wait/notify.
>
>
> This addresses bug SQOOP-690.
>     https://issues.apache.org/jira/browse/SQOOP-690
>
>
> Diffs
> -----
>
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/io/Data.java 41fceb8
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java 0d636ae
>
> Diff: https://reviews.apache.org/r/8063/diff/
>
>
> Testing
> -------
>
> Ran unit tests, and on a real cluster.
>
>
> Thanks,
>
> Hari Shreedharan
>
>

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

(Updated Nov. 16, 2012, 6:15 p.m.)
Review request for Sqoop.
Description
-------

Improved thread handling in SqoopOutputFormatLoadExecutor. Removed the synchronized blocks and wait/notify.
This addresses bug SQOOP-690.
    https://issues.apache.org/jira/browse/SQOOP-690
Diffs (updated)
-----

  connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportLoader.java ff7384c
  execution/mapreduce/src/main/java/org/apache/sqoop/job/etl/HdfsSequenceImportLoader.java 29a73b0
  execution/mapreduce/src/main/java/org/apache/sqoop/job/etl/HdfsTextImportLoader.java 711df0f
  execution/mapreduce/src/main/java/org/apache/sqoop/job/io/Data.java 41fceb8
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java 0d636ae
  execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 6dcf784
  spi/src/main/java/org/apache/sqoop/job/etl/Loader.java 3a708df
  spi/src/main/java/org/apache/sqoop/job/io/DataReader.java 18e2fb7

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

Ran unit tests, and on a real cluster.
Thanks,

Hari Shreedharan

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

Ship it!
Patch looks good to me, thank you Hari for your time.

- Jarek Cecho
On Nov. 16, 2012, 6:15 p.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8063/
> -----------------------------------------------------------
>
> (Updated Nov. 16, 2012, 6:15 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Improved thread handling in SqoopOutputFormatLoadExecutor. Removed the synchronized blocks and wait/notify.
>
>
> This addresses bug SQOOP-690.
>     https://issues.apache.org/jira/browse/SQOOP-690
>
>
> Diffs
> -----
>
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportLoader.java ff7384c
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/etl/HdfsSequenceImportLoader.java 29a73b0
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/etl/HdfsTextImportLoader.java 711df0f
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/io/Data.java 41fceb8
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java 0d636ae
>   execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 6dcf784
>   spi/src/main/java/org/apache/sqoop/job/etl/Loader.java 3a708df
>   spi/src/main/java/org/apache/sqoop/job/io/DataReader.java 18e2fb7
>
> Diff: https://reviews.apache.org/r/8063/diff/
>
>
> Testing
> -------
>
> Ran unit tests, and on a real cluster.
>
>
> Thanks,
>
> Hari Shreedharan
>
>