Vuk Ercegovac 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 5: (9 comments) mostly clarifying comments, otherwise, looks good. 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 ... all impalad coordinators ... while "full" seems to be legal with a mix of v1/v2, worth stating that the complete objects will be a waste for v2 impalads? hmm.. just read where this flag is used. the precondition implies that minimal/mixed is required v2 (getPartial) rather than an optimization to avoid sending complete objects to v2 coordinators. http://gerrit.cloudera.org:8080/#/c/11280/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/11280/5/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@712 PS5, Line 712: if (obj.type == TCatalogObjectType.CATALOG) { if this is a statestore-update-only branch, please add a comment. http://gerrit.cloudera.org:8080/#/c/11280/5/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@737 PS5, Line 737: minVersion per the interface comment, the return here is to "implement SYNC_DDL". here, the limitation is described to pertain to only 'invalidate metadata' + sync_ddl, which -1 is used to disable. Since the return is used for all statestore-related updates, how does -1 interact with operations that are not invalidate metdata + sync_ddl? http://gerrit.cloudera.org:8080/#/c/11280/5/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@738 PS5, Line 738: return add a reminder that this is ignored for the ddl case. http://gerrit.cloudera.org:8080/#/c/11280/5/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@745 PS5, Line 745: different places what does this mean? http://gerrit.cloudera.org:8080/#/c/11280/5/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@763 PS5, Line 763: // an issue in practice, so deferring it to later clean-up. perhaps associate the catalog service id with the object, so that it can be tested whether or not an object is in the cache from a different catalogd incarnation. http://gerrit.cloudera.org:8080/#/c/11280/5/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@967 PS5, Line 967: final long version_; so for the current scheme, if a table comment is modified, its version will get updated, the statestore will pass that around, and this will be invalidated (or simply not found)? http://gerrit.cloudera.org:8080/#/c/11280/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/11280/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@943 PS5, Line 943: // TODO(todd): if client is a 'v2' impalad, only send back invalidation just to make sure I understand this, but the object handled here will be sent to statestore in minimized form? if so, can the proposed change be handled entirely by the method that gets the minimal object (in CatalogServiceCatalog)? http://gerrit.cloudera.org:8080/#/c/11280/5/tests/custom_cluster/test_local_catalog.py File tests/custom_cluster/test_local_catalog.py: http://gerrit.cloudera.org:8080/#/c/11280/5/tests/custom_cluster/test_local_catalog.py@86 PS5, Line 86: self.cluster.catalogd.kill() I don't think its an issue any longer, but just checking if this results in zombie processes, which then messes with subsequent custom cluster tests that restart the cluster. Before, it showed up when running all customer cluster tests. -- 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: 5 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: Fri, 31 Aug 2018 20:58:22 +0000 Gerrit-HasComments: Yes
