> On Sept. 24, 2015, 8:50 p.m., Jarek Cecho wrote: > > Overall the code looks good to me, but I have few questions: > > > > 1) Since this is significant change, we should provide some tests for it. > > I'm not immediately sure how to test those though. Any thoughts Dian? > > > > 2) I was wondering what are we gainging by caching the loaded classes? Is > > it faster then without the caching? > > > > 3) I didn't fully understood the need for NEGATIVE_CACHE_SENTINEL - can't > > be "null" used instead of it? > > > > Jarcec > > Dian Fu wrote: > Thanks Jarcec for the review. > 1) For the tests, I have updated the patch adding one test case. But this > test case has dependency to SQOOP-2578, so I have canceled the patch > available state of this JIRA and will set it back when SQOOP-2578 is in. > 2) For caching the classes and NEGATIVE_CACHE_SENTINEL, in fact I > borrowed that implementation from Hadoop. Caching will be faster(refer to > HADOOP-6133 for details), but I think that class loading time won't be a > problem for SQOOP 2 and so I'm ok to remove the cache. But on the other side, > keeping it there should have no harm. :) > 3) If you agree to keep cache there, then NEGATIVE_CACHE_SENTINEL will be > necessary. (HADOOP-8157 has detailed discussion on this)
Thanks for all the pointers Dian! It was helpful for me to go through the comments to understand the reasoning behind those. 1) Thanks for the rudimentary test. It's not covering all the options, but I think that the rest of the code will excercise it appropriately. Hence I'm fine with this. 2) + 3) The pointers to Hadoop JIRAs are extremely helpful. Consider my points as moot :) I'll finish the review and hopefully commit this soon. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38670/#review100461 ----------------------------------------------------------- On Sept. 25, 2015, 5:41 a.m., Dian Fu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38670/ > ----------------------------------------------------------- > > (Updated Sept. 25, 2015, 5:41 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2577 > https://issues.apache.org/jira/browse/SQOOP-2577 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > Currently, ClassUtils doesn't support to load classes using a custom > classloader. The aim of this JIRA is to provide ClassUtils with this capacity. > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/utils/ClassUtils.java 6262802 > common/src/test/java/org/apache/sqoop/utils/TestClassUtils.java 4126e7b > > Diff: https://reviews.apache.org/r/38670/diff/ > > > Testing > ------- > > > Thanks, > > Dian Fu > >
