----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6114/#review9547 -----------------------------------------------------------
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. http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/common/HCatConf.java <https://reviews.apache.org/r/6114/#comment20387> Why not put this in HCatConstants? Let's worry about an HCatConf class when we really need it. http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/common/HCatConf.java <https://reviews.apache.org/r/6114/#comment20388> typo, comma instead of period http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/common/HCatUtil.java <https://reviews.apache.org/r/6114/#comment20406> 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? http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/common/HCatUtil.java <https://reviews.apache.org/r/6114/#comment20389> Catching exceptions should be enough. Errors are describe as "serious problems" potentially problems with the JVM. So masking that error and plodding along is a bad idea. http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/common/cache/HiveClientCache.java <https://reviews.apache.org/r/6114/#comment20410> typo http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/common/cache/HiveClientCache.java <https://reviews.apache.org/r/6114/#comment20407> nitpick: why not use elements.values() instead? So you don't need the lookup. http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/common/cache/HiveClientCache.java <https://reviews.apache.org/r/6114/#comment20408> Just catch exception http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/common/cache/HiveClientCache.java <https://reviews.apache.org/r/6114/#comment20409> This should be WARN http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/common/cache/HiveClientCache.java <https://reviews.apache.org/r/6114/#comment20412> Is this method atomic? if not you might what to make the enclosing method sychronized. http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/common/cache/HiveClientCache.java <https://reviews.apache.org/r/6114/#comment20411> Wouldn't it be better to move this into call()? http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/mapreduce/FileOutputCommitterContainer.java <https://reviews.apache.org/r/6114/#comment20394> why skip closing the connection? When it's being close everywhere else? http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/pig/PigHCatUtil.java <https://reviews.apache.org/r/6114/#comment20390> why not reuse HCatUtil.getHiveClient()? - Francis Liu 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 > >
