Todd Lipcon 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: (10 comments) http://gerrit.cloudera.org:8080/#/c/11280/5/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/11280/5/be/src/catalog/catalog-server.cc@49 PS5, Line 49: impalads > My suggestion is to make it clear what should be used when. I assume that t Done 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 Done 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. Done http://gerrit.cloudera.org:8080/#/c/11280/5/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/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1996 PS5, Line 1996: TCatalogObject objectDesc = Preconditions.checkNotNull(req.object_desc, > I recall this issue, MPALA-4704 where we now wait until an update is receiv It's actually that, in the default dev environment, we'll be running catalogd in "full" mode (v1-only). But, PartialCatalogTest is actually making RPCs to the running catalogd (not in-process) and therefore it was getting rejected. Given that it's perfectly _safe_ to respond to these RPCs regardless of the topic configuration, I elected to remove this. 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_ == Done 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 th Done 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 Done 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? Done 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? Done 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? Done. I tried to use a test dimension but sadly doesn't seem like dimensions can be accessed from within 'setup_method', so had to do a bit of ugly copy-paste. oh well... -- 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 <[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, 04 Sep 2018 18:18:44 +0000 Gerrit-HasComments: Yes
