Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11208 )
Change subject: IMPALA-7437. LRU caching of partitions in impalad ...................................................................... Patch Set 8: (8 comments) Just a bunch of nits I found skimming through the change. Feel free to address in the following changes. Change lgtm. http://gerrit.cloudera.org:8080/#/c/11208/8/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/8/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@281 PS8, Line 281: if (!missingCols.isEmpty()) { nit: May be easier to read if we invert the condition. if (missintCols.isEmpty()) return emptyList; http://gerrit.cloudera.org:8080/#/c/11208/8/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@286 PS8, Line 286: "); Add table name? http://gerrit.cloudera.org:8080/#/c/11208/8/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@324 PS8, Line 324: "); table name http://gerrit.cloudera.org:8080/#/c/11208/8/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@329 PS8, Line 329: ); table http://gerrit.cloudera.org:8080/#/c/11208/8/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@415 PS8, Line 415: "); table + part name http://gerrit.cloudera.org:8080/#/c/11208/8/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@435 PS8, Line 435: ); , tablename? http://gerrit.cloudera.org:8080/#/c/11208/8/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@492 PS8, Line 492: throw new TException(String.format("Invalid response from catalogd for request " + Should we log the exception stack here instead of logging it at all the callers? http://gerrit.cloudera.org:8080/#/c/11208/8/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@689 PS8, Line 689: * Cache key for the partition list of a table. nit:For a moment I got confused if there is duplication between this and PartitionCacheKey. May be good to clarify that they cache different forms of Partition metadata -- 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: 8 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: Wed, 22 Aug 2018 21:20:35 +0000 Gerrit-HasComments: Yes
