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