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