> 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. > > Arup Malakar wrote: > 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. > > Rohini Palaniswamy wrote: > According to https://issues.apache.org/jira/browse/HIVE-84 the > HiveMetastoreClient is threadsafe and they are internally using threadlocal > looking at the patch. May be that has changed. Ashutosh might know better. > But I have used multiple instances of HiveMetastoreClient in different > threads for two different servers when I wrote the HA testsuite and there > were no issues. But then I only did getDatabases and getTables in it. The > Driver class I have heard is not thread safe though. Can you send a mail to > Ashutosh and check?
Hmm I read the Jira differently. Comment #1 mentions "Metastore Client (s0 Hive.java) is never intended to be thread-safe" and looking at the patch it made no changes to metastore client but instead made it threadlocal. I pinged Ashutosh He doesn't have an answer. Sent an email to dev@hive. Will post back what I get. > 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? > > Arup Malakar wrote: > 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. I'm for removing the option and just properly ironing out the bugs (if any). > 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()? > > Arup Malakar wrote: > 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." > I see, thanks for the clarification. Though it seems like you need the acquire and get() to be atomic. Though let's sit on that for now as we might not need any of these concurrency control mechanisms if we're going with thread local. > 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. > > Arup Malakar wrote: > 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." I see, it does sound that way. - Francis ----------------------------------------------------------- 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 > >
