Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11280 )
Change subject: IMPALA-7469. Invalidate LocalCatalog cache based on topic updates ...................................................................... Patch Set 7: Code-Review+1 (8 comments) The overall code flow and caching structure makes sense to me. I'm sure there will be some races around cache invalidations that can surface during load tests but they can be fixed as follow up patches. Feel free to carry my +1 after you fix the nits that you find relevant. http://gerrit.cloudera.org:8080/#/c/11280/7/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/11280/7/be/src/catalog/catalog-server.cc@44 PS7, Line 44: Th nit: Maybe mention that the valid values here are "full", "mixed", "minimal"... http://gerrit.cloudera.org:8080/#/c/11280/7/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java File fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java: http://gerrit.cloudera.org:8080/#/c/11280/7/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@124 PS7, Line 124: i Gives an impression that invalidate metadata in any form is not supported. Explicitly mention global? http://gerrit.cloudera.org:8080/#/c/11280/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/11280/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@456 PS7, Line 456: TCatalogObject min = new TCatalogObject(obj.type, obj.catalog_version); Preconditions.checkState((topicMode_ == TopicMode.MINIMAL || topicMode_ == TopicMode.MIXED) )? http://gerrit.cloudera.org:8080/#/c/11280/7/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/11280/7/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@190 PS7, Line 190: */ Can you add some detail about how SYNC_DDL is done with this? Would help the readers. http://gerrit.cloudera.org:8080/#/c/11280/7/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@711 PS7, Line 711: extra \n http://gerrit.cloudera.org:8080/#/c/11280/7/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@746 PS7, Line 746: catalogServiceId_ Need some synchronization on catalogServiceIdLock_ just to be safe? http://gerrit.cloudera.org:8080/#/c/11280/7/fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoTest.java File fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoTest.java: http://gerrit.cloudera.org:8080/#/c/11280/7/fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoTest.java@29 PS7, Line 29: import org.apache.impala.common.RuntimeEnv; Remove? http://gerrit.cloudera.org:8080/#/c/11280/7/tests/custom_cluster/test_local_catalog.py File tests/custom_cluster/test_local_catalog.py: http://gerrit.cloudera.org:8080/#/c/11280/7/tests/custom_cluster/test_local_catalog.py@25 PS7, Line 25: Should we have any test coverage for mixed mode catalog here? -- To view, visit http://gerrit.cloudera.org:8080/11280 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I615f9e6bd167b36cd8d93da59426dd6813ae4984 Gerrit-Change-Number: 11280 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Sat, 01 Sep 2018 03:35:51 +0000 Gerrit-HasComments: Yes
