Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11208 )
Change subject: IMPALA-7437. LRU caching of partitions in impalad ...................................................................... Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/11208/5/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/11208/5/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@235 PS5, Line 235: List<ColumnStatisticsObj> ret = Lists.newArrayListWithCapacity(colNames.size()); > Yea, I was going to defer this and try to add a somewhat generic "request m makes sense. http://gerrit.cloudera.org:8080/#/c/11208/5/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@268 PS5, Line 268: ret.size() > yea, I actually ended up implementing the negative cache in a later patch i sg, thx! http://gerrit.cloudera.org:8080/#/c/11208/5/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@280 PS5, Line 280: new Callable<List<PartitionRef>>() { perhaps add a comment here about this being the miss handler. http://gerrit.cloudera.org:8080/#/c/11208/5/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@660 PS5, Line 660: private final long partId_; nit: add newline to be consistent with other classes. http://gerrit.cloudera.org:8080/#/c/11208/5/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java File fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java: http://gerrit.cloudera.org:8080/#/c/11208/5/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java@69 PS5, Line 69: public void testCachePartitions() throws Exception { perhaps add the suffix "ByRefs" so its clearer that this test is aimed at the loadPartitionsByRefs method (unless I've misunderstood it). http://gerrit.cloudera.org:8080/#/c/11208/5/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java@71 PS5, Line 71: List<PartitionRef> refs = allRefs.subList(3, 8); clearer to call this partialRefs http://gerrit.cloudera.org:8080/#/c/11208/5/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java@95 PS5, Line 95: } add a test for loadTableColumnStatistics -- To view, visit http://gerrit.cloudera.org:8080/11208 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9a57521ad851da605604a1e7c48d3d6627da5df5 Gerrit-Change-Number: 11208 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Tue, 21 Aug 2018 00:10:59 +0000 Gerrit-HasComments: Yes
