> On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java > > Lines 63 (patched) > > <https://reviews.apache.org/r/65634/diff/4/?file=1968309#file1968309line63> > > > > It would be cleaner and easier to read to rewrite this as > > > > ``` > > public static String buildKey(List<String> partVals) { > > if (partVals == null || partVals.isEmpty()) { > > return ""; > > } > > return String.join(delimit, partVals); > > } > > ```
I have made the change based on your suggestion, but I think it's just a preference of style. > On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java > > Lines 70 (patched) > > <https://reviews.apache.org/r/65634/diff/4/?file=1968309#file1968309line70> > > > > 1) Please add Javadoc comment, explaining what this function does. > > 2) Is overloading really useful here? I agree, makes sense to have a new method than overload since the params don't really convey the intent if you overlook the param name. I'll make changes to other methods as well. > On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java > > Lines 71 (patched) > > <https://reviews.apache.org/r/65634/diff/4/?file=1968309#file1968309line71> > > > > why not just > > > > `return buildKey(partVals) + delimit + colName` > > > > can colName be empty here or not? colName won't be empty here. > On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java > > Lines 128 (patched) > > <https://reviews.apache.org/r/65634/diff/4/?file=1968310#file1968310line147> > > > > 1) Please add units in the name and use constant for the default value. > > 2) Please document what is `cacheRefreshPeriod`. It is already documented as part of the Conf class. Adding the same note here. > On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java > > Line 226 (original), 140 (patched) > > <https://reviews.apache.org/r/65634/diff/4/?file=1968310#file1968310line229> > > > > Please document this method - in particular how does it gets cache > > implementaiton from config. Actually I don't think this method is needed now (have removed it). This was introduced in HIVE-17629, when prewarm was a blocking call. In this patch, I have made it non-blocking and it runs in a background thread so that metastore can remain usable during cache prewarm. > On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java > > Line 262 (original), 175 (patched) > > <https://reviews.apache.org/r/65634/diff/4/?file=1968310#file1968310line265> > > > > This doesn't look correct: > > > > 1) initBlackListWhiteList() will not update any existing whitelist or > > blacklist, only add one if it wasn't there. > > 2) initBlackListWhiteList() is calling > > `Collections.reverse(blacklistPatterns)` which doesn't make sense when > > configuration is set to a new value. This is picked once only when the metastore server is started and then used thereafter. Removed Collections.reverse. > On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java > > Lines 176 (patched) > > <https://reviews.apache.org/r/65634/diff/4/?file=1968310#file1968310line266> > > > > Looks like every time someone calls setConf() a new thread is started - > > isn't it a threda leak? > > In general it isn't a good practice to add such side-effects for config > > changes like setConf - it is better to explicitly call a method which will > > do whatever is needed after conf update. No, a new thread is started only when cacheUpdateMaster is null or from the Unit tests (where we explicitly try to control start and stop, and where the thread dies after one run). > On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java > > Line 266 (original), 202 (patched) > > <https://reviews.apache.org/r/65634/diff/4/?file=1968310#file1968310line292> > > > > Please document this method. Among other things - can prewarm() be > > called multiple times? If not, should it be somehow enforced? Have enforced that. Currently it was being called just once from the background thread. > On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java > > Line 271 (original), 207 (patched) > > <https://reviews.apache.org/r/65634/diff/4/?file=1968310#file1968310line297> > > > > Please remove <Database> and add explicit size: > > > > `List<Database> databases = new ArrayList<>(dbNames.size());` I had kept it for Java 6 compatibility, but looks like Hive2+ doesn't support it. Removed > On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java > > Line 285 (original), 217 (patched) > > <https://reviews.apache.org/r/65634/diff/4/?file=1968310#file1968310line318> > > > > It is quite possible that there is another metastore instance running > > and someone removes this database, so this call will fail due to missing > > database. I think this code should continue prewarm for other databases in > > such cases. rawStore.getAllTables(dbName) should return an empty list, so processing will continue. > On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java > > Line 368 (original), 305 (patched) > > <https://reviews.apache.org/r/65634/diff/4/?file=1968310#file1968310line419> > > > > ANy reason not to use lambda here? Not really, just a matter of preference here. > On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java > > Line 746 (original), 595 (patched) > > <https://reviews.apache.org/r/65634/diff/4/?file=1968310#file1968310line853> > > > > Wouldn;t dropDatabase also throw exception if it fails? Yes, we're throwing it if we get an exception from rawStore.dropDatabase > On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java > > Line 758 (original), 599 (patched) > > <https://reviews.apache.org/r/65634/diff/4/?file=1968310#file1968310line865> > > > > This is rather useless since in case of failure tu'll throw > > MetaException anyway. Here we are working with the Rawstore API which returns a boolean which can potentially be false. In that case we don't want to work on SharedCache. The underlying ObjectStore implementation may change in future > On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java > > Line 902 (original), 701 (patched) > > <https://reviews.apache.org/r/65634/diff/4/?file=1968310#file1968310line1013> > > > > tbl would never be null, there will be an exception if the call above > > fails. If the table is not yet in the cache (cache not prewarmed yet), sharedCache.getTableFromCache will return null. In that case we would like to serve the call from the metastore db. - Vaibhav ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65634/#review198507 ----------------------------------------------------------- On March 1, 2018, 11:09 a.m., Vaibhav Gumashta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65634/ > ----------------------------------------------------------- > > (Updated March 1, 2018, 11:09 a.m.) > > > Review request for hive, Daniel Dai and Thejas Nair. > > > Bugs: HIVE-18264 > https://issues.apache.org/jira/browse/HIVE-18264 > > > Repository: hive-git > > > Description > ------- > > https://issues.apache.org/jira/browse/HIVE-18264 > > > Diffs > ----- > > > itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java > a3725c5395 > service/src/java/org/apache/hive/service/server/HiveServer2.java 86c9c2b33c > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > ac71d0882f > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > 7b44df4128 > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java > f500d63725 > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java > f0f650ddcf > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java > 0d132f2074 > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/SharedCache.java > 32ea17495f > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java > 50f873a013 > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java > 75ea8c4a77 > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java > 207d842f94 > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/cache/TestCachedStore.java > ab6feb6f0b > standalone-metastore/src/test/resources/log4j2.properties 365687e1c9 > > > Diff: https://reviews.apache.org/r/65634/diff/4/ > > > Testing > ------- > > > Thanks, > > Vaibhav Gumashta > >
