----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62228/#review185136 -----------------------------------------------------------
metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java Lines 1432-1433 (original) <https://reviews.apache.org/r/62228/#comment261385> It will be useful to retain (improved) comment. metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java Line 1977 (original), 1977 (patched) <https://reviews.apache.org/r/62228/#comment261386> Better comment: Group stats by colName for each partition metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java Lines 1993 (patched) <https://reviews.apache.org/r/62228/#comment261389> LOG.debug metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java Line 1999 (original), 2007 (patched) <https://reviews.apache.org/r/62228/#comment261390> for number of threads, better logic could be Math.min(colStatsMap.size(), Runtime.getRuntime().availableProcessors()) metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java Line 2008 (original), 2010 (patched) <https://reviews.apache.org/r/62228/#comment261391> LOG.debug metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java Lines 2024 (patched) <https://reviews.apache.org/r/62228/#comment261392> Can remove this metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java Lines 2025 (patched) <https://reviews.apache.org/r/62228/#comment261393> LOG.debug(e.getMessage()) metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java Line 2025 (original), 2035 (patched) <https://reviews.apache.org/r/62228/#comment261394> future will never be null metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java Line 2027 (original), 2039 (patched) <https://reviews.apache.org/r/62228/#comment261395> Better to keep pool.shutdownNow() and remove e.printsTacktrace() metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java Lines 2045 (patched) <https://reviews.apache.org/r/62228/#comment261396> LOG.debug metastore/src/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java Lines 275 (patched) <https://reviews.apache.org/r/62228/#comment261397> Here we are listing all partitions for table and than we immediately aggr stats for all partitions. Another (better) way is to not retrieve partNames and do a sql query to aggr stats for partitions by partFilterExpr. Essentially get_aggr_stats_for(dbName, tblName, partFilterExpr). Here, partFilterExpr = * That will allow many roundtrips to backend DB. metastore/src/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java Lines 295 (patched) <https://reviews.apache.org/r/62228/#comment261398> And here it will be partFilterExpr = partNames not in (defaultPartition) metastore/src/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java Line 267 (original), 301 (patched) <https://reviews.apache.org/r/62228/#comment261400> Useful to log time taken to prewarm: LOG.info("Time taken to prewarm: " + (System.currentTimeMillis()-start)/1000); metastore/src/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java Line 1535 (original), 1569 (patched) <https://reviews.apache.org/r/62228/#comment261401> Caller from CachedStore made this call before calling this method. Might as well pass from there. metastore/src/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java Line 1536 (original), 1570 (patched) <https://reviews.apache.org/r/62228/#comment261402> This if condition will always be true for cachedstore prewarm invocation. can you please add comments for that. metastore/src/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java Lines 1588 (patched) <https://reviews.apache.org/r/62228/#comment261404> LOG.debug metastore/src/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java Line 1563 (original), 1623 (patched) <https://reviews.apache.org/r/62228/#comment261405> LOG.debug and Previous construction of msg was better. metastore/src/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java Lines 1630 (patched) <https://reviews.apache.org/r/62228/#comment261407> should this be < 1 instead of <=1 ? metastore/src/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java Lines 1631 (patched) <https://reviews.apache.org/r/62228/#comment261406> LOG.debug with {} instead of + metastore/src/java/org/apache/hadoop/hive/metastore/cache/SharedCache.java Lines 461 (patched) <https://reviews.apache.org/r/62228/#comment261408> {} instead of + metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/ColumnStatsAggregatorFactory.java Lines 44 (patched) <https://reviews.apache.org/r/62228/#comment261410> LOG.trace metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/ColumnStatsAggregatorFactory.java Lines 44 (patched) <https://reviews.apache.org/r/62228/#comment261411> LOG.trace metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/ColumnStatsAggregatorFactory.java Line 82 (original), 88 (patched) <https://reviews.apache.org/r/62228/#comment261412> Please don't make this change. This is done so that we only do deserialization once for bit vector. This change will loose perf. metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/ColumnStatsAggregatorFactory.java Line 86 (original), 92 (patched) <https://reviews.apache.org/r/62228/#comment261413> Please don't make this change. This is done so that we only do deserialization once for bit vector. This change will loose perf. metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/ColumnStatsAggregatorFactory.java Line 90 (original), 96 (patched) <https://reviews.apache.org/r/62228/#comment261414> Please don't make this change. This is done so that we only do deserialization once for bit vector. This change will loose perf. metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/ColumnStatsAggregatorFactory.java Line 94 (original), 100 (patched) <https://reviews.apache.org/r/62228/#comment261415> Please don't make this change. This is done so that we only do deserialization once for bit vector. This change will loose perf. metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/ColumnStatsAggregatorFactory.java Line 102 (original), 108 (patched) <https://reviews.apache.org/r/62228/#comment261416> Please don't make this change. This is done so that we only do deserialization once for bit vector. This change will loose perf. metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/DateColumnStatsAggregator.java Line 53 (original) <https://reviews.apache.org/r/62228/#comment261418> I think this way of determining whether all parts have stats or not is better than passing areAllPartsFound variable which is computed elsewhere. metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/DateColumnStatsAggregator.java Lines 82 (patched) <https://reviews.apache.org/r/62228/#comment261417> LOG.trace metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/DateColumnStatsAggregator.java Line 236 (original), 231 (patched) <https://reviews.apache.org/r/62228/#comment261419> debug with {} metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/DecimalColumnStatsAggregator.java Line 104 (original) <https://reviews.apache.org/r/62228/#comment261420> Aren't we expecting ColumnStatsDataInspector here? metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/DecimalColumnStatsAggregator.java Line 193 (original) <https://reviews.apache.org/r/62228/#comment261421> we shall expect inspector here. metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/DecimalColumnStatsAggregator.java Line 257 (original), 259 (patched) <https://reviews.apache.org/r/62228/#comment261422> Debug with {} metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/DoubleColumnStatsAggregator.java Line 52 (original) <https://reviews.apache.org/r/62228/#comment261423> This logic is better than areAllPartsFound passed in. metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/DoubleColumnStatsAggregator.java Line 68 (original), 60 (patched) <https://reviews.apache.org/r/62228/#comment261424> Need inspectors here. metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/DoubleColumnStatsAggregator.java Line 101 (original), 94 (patched) <https://reviews.apache.org/r/62228/#comment261425> inspector is expected metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/DoubleColumnStatsAggregator.java Line 232 (original), 228 (patched) <https://reviews.apache.org/r/62228/#comment261426> debug metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/DoubleColumnStatsAggregator.java Line 243 (original), 240 (patched) <https://reviews.apache.org/r/62228/#comment261427> Please don't make this change. metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/LongColumnStatsAggregator.java Lines 229 (patched) <https://reviews.apache.org/r/62228/#comment261428> debug with {} metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/StringColumnStatsAggregator.java Line 154 (original), 147 (patched) <https://reviews.apache.org/r/62228/#comment261429> inspector expected. metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/StringColumnStatsAggregator.java Line 204 (original), 197 (patched) <https://reviews.apache.org/r/62228/#comment261430> LOG.debug with {} - Ashutosh Chauhan On Sept. 11, 2017, 9:25 p.m., Vaibhav Gumashta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62228/ > ----------------------------------------------------------- > > (Updated Sept. 11, 2017, 9:25 p.m.) > > > Review request for hive, Ashutosh Chauhan and Thejas Nair. > > > Bugs: HIVE-17495 > https://issues.apache.org/jira/browse/HIVE-17495 > > > Repository: hive-git > > > Description > ------- > > https://issues.apache.org/jira/browse/HIVE-17495 > > > Diffs > ----- > > > itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java > 8d861e4 > metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java > dc1245e > metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java > bbe13fd > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java > 3053dcb > metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 71982a0 > metastore/src/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java > 3ba81ce > metastore/src/java/org/apache/hadoop/hive/metastore/cache/SharedCache.java > 80b17e0 > > metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/BinaryColumnStatsAggregator.java > e6c836b > > metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/BooleanColumnStatsAggregator.java > a34bc9f > > metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/ColumnStatsAggregator.java > a52e5e5 > > metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/ColumnStatsAggregatorFactory.java > dfae708 > > metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/DateColumnStatsAggregator.java > ee95396 > > metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/DecimalColumnStatsAggregator.java > 284c12c > > metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/DoubleColumnStatsAggregator.java > bb4a725 > > metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/LongColumnStatsAggregator.java > 5b1145e > > metastore/src/java/org/apache/hadoop/hive/metastore/columnstats/aggr/StringColumnStatsAggregator.java > 1b29f92 > > metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java > 4db203d > > metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java > fb16cfc > > > Diff: https://reviews.apache.org/r/62228/diff/1/ > > > Testing > ------- > > > Thanks, > > Vaibhav Gumashta > >
