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