> On July 27, 2012, 10:09 p.m., Francis Liu wrote: > > First pass skipped a few things but wanted to give early feedback. My main > > concern is that I don't think HiveMetaStoreClient is threadsafe as the > > thrift client is not? If that is the case then having the cache thread > > local will be a cleaner approach.
If HiveMetaStoreClient cannot be shared across threads then what you say is right. Rohini, what is your opinion? I have addressed the rest of the comments (few of them were already taken care of in the 5th version: https://reviews.apache.org/r/6114/diff/5/ I will update the review incorporating the "ThreadLocalality" once Rohini gives her feedback. > On July 27, 2012, 10:09 p.m., Francis Liu wrote: > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/common/cache/HiveClientCache.java, > > line 126 > > <https://reviews.apache.org/r/6114/diff/3/?file=130091#file130091line126> > > > > Wouldn't it be better to move this into call()? If I understand correctly, the call() would be called only if there aren't any valid entry in the cache. So if the cache returns existing item it wont be called. We want acquire() to bookkeep existing users. so need to call acquire() in both cases (creation or returning existing cached item). Quoting: http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/cache/Cache.html#get(java.lang.Object,java.util.concurrent.Callable) "Returns the value associated with key in this cache, obtaining that value from valueLoader if necessary. No observable state associated with this cache is modified until loading completes. This method provides a simple substitute for the conventional "if cached, return; otherwise create, cache and return" pattern." > On July 27, 2012, 10:09 p.m., Francis Liu wrote: > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/common/cache/HiveClientCache.java, > > line 120 > > <https://reviews.apache.org/r/6114/diff/3/?file=130091#file130091line120> > > > > Is this method atomic? if not you might what to make the enclosing > > method sychronized. Guava docs says these methods are thread-safe implying two instances wont get created (my interpretation, haven't checked their implementation). http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/cache/Cache.html "Implementations of this interface are expected to be thread-safe, and can be safely accessed by multiple concurrent threads." > On July 27, 2012, 10:09 p.m., Francis Liu wrote: > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/common/HCatUtil.java, > > line 531 > > <https://reviews.apache.org/r/6114/diff/3/?file=130090#file130090line531> > > > > Apart from the implementation potentially having bugs, what reason > > would someone not to use caching? > > > > If there's no good reason then there's no need to have "no caching" as > > an option? Rohini suggested we keep this configuration initially, so that people would have the option of disabling it if there is any bug found in the implementation. That way it can be disabled rather than rolling a build to fix the bug. Once proven we can and should get rid of this configuration. But I'm open to removing this if you feel we don't need this configuration. - Arup ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6114/#review9547 ----------------------------------------------------------- On July 27, 2012, 8:38 p.m., Arup Malakar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6114/ > ----------------------------------------------------------- > > (Updated July 27, 2012, 8:38 p.m.) > > > Review request for hcatalog and Rohini Palaniswamy. > > > Description > ------- > > Time expiry based HiveMetaStoreClient cache for HCatalog > > > This addresses bug HCATALOG-370. > https://issues.apache.org/jira/browse/HCATALOG-370 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/ivy.xml > 1364830 > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/common/HCatConf.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/common/HCatUtil.java > 1364830 > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/common/cache/HiveClientCache.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/mapreduce/DefaultOutputCommitterContainer.java > 1364830 > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/mapreduce/FileOutputCommitterContainer.java > 1364830 > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/mapreduce/FileOutputFormatContainer.java > 1364830 > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/mapreduce/HCatOutputFormat.java > 1364830 > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/mapreduce/InitializeInput.java > 1364830 > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/pig/PigHCatUtil.java > 1364830 > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/test/org/apache/hcatalog/common/TestHiveClientCache.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/6114/diff/ > > > Testing > ------- > > > Thanks, > > Arup Malakar > >
