Hari Shreedharan
2012-11-15, 01:59
Juhani Connolly
2012-11-16, 02:03
Jarek Jarcec Cecho
2012-11-16, 02:11
Hari Shreedharan
2012-11-16, 09:48
Jarek Cecho
2012-11-16, 17:22
Hari Shreedharan
2012-11-16, 18:15
Hari Shreedharan
2012-11-16, 18:15
Jarek Cecho
2012-11-16, 20:09
-
Re: Review Request: SQOOP-690. Fix threading issues in SqoopOutputFormatLoadExecutorHari Shreedharan 2012-11-15, 01:59
-----------------------------------------------------------
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
-
Re: Review Request: SQOOP-690. Fix threading issues in SqoopOutputFormatLoadExecutorJuhani Connolly 2012-11-16, 02:03
> 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
>
>
-
Re: Review Request: SQOOP-690. Fix threading issues in SqoopOutputFormatLoadExecutorJarek Jarcec Cecho 2012-11-16, 02:11
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
> >
> >
>
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
> >
> >
>
-
Re: Review Request: SQOOP-690. Fix threading issues in SqoopOutputFormatLoadExecutorHari Shreedharan 2012-11-16, 09:48
-----------------------------------------------------------
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
-
Re: Review Request: SQOOP-690. Fix threading issues in SqoopOutputFormatLoadExecutorJarek Cecho 2012-11-16, 17:22
-----------------------------------------------------------
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
>
>
-
Re: Review Request: SQOOP-690. Fix threading issues in SqoopOutputFormatLoadExecutorHari Shreedharan 2012-11-16, 18:15
> 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
>
>
-
Re: Review Request: SQOOP-690. Fix threading issues in SqoopOutputFormatLoadExecutorHari Shreedharan 2012-11-16, 18:15
-----------------------------------------------------------
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
-
Re: Review Request: SQOOP-690. Fix threading issues in SqoopOutputFormatLoadExecutorJarek Cecho 2012-11-16, 20:09
-----------------------------------------------------------
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
>
>
