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 2: (13 comments) haven't looked at the tests yet. http://gerrit.cloudera.org:8080/#/c/11280/2/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/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@422 PS2, Line 422: : // Serialize a minimal version of the object that can be used by impalads : // that are running in 'local-catalog' mode. This is used by those impalads : // to invalidate their local cache. : TCatalogObject minimalObject = getMinimalObjectForV2(obj); : String v2Key = CatalogObjectsConstants.CATALOG_TOPIC_V2_PREFIX + key; : if (!FeSupport.NativeAddPendingTopicItem(nativeCatalogServerPtr, v2Key, : obj.catalog_version, serializer.serialize(minimalObject), delete)) { : LOG.error("NativeAddPendingTopicItem failed in BE. key=" + v2Key + ", delete=" : + delete + ", data_size=" + data.length); : } Can we avoid these if there are no subscribers subscribed to this prefix? (which I believe is the most common case until the local catalog is a stable feature) Please ignore if it annoying to plumb through the code to get this information. http://gerrit.cloudera.org:8080/#/c/11280/2/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/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@255 PS2, Line 255: public Database loadDb(final String dbName) throws TException { nit: Add method docs in general. Also worth mentioning that it tries to get from cache, else makes an RPC .. http://gerrit.cloudera.org:8080/#/c/11280/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@263 PS2, Line 263: TGetPartialCatalogObjectRequest req = newReqForDb(dbName); hit.setRef(false) (below too), wondering if can make this whole logic of cache get/call and set hit bit into a method. Its all over the place. http://gerrit.cloudera.org:8080/#/c/11280/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@267 PS2, Line 267: ); dbname http://gerrit.cloudera.org:8080/#/c/11280/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@290 PS2, Line 290: TGetPartialCatalogObjectRequest req = newReqForDb(dbName); hit.setRef(false) http://gerrit.cloudera.org:8080/#/c/11280/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@294 PS2, Line 294: expected HMS table" table names? http://gerrit.cloudera.org:8080/#/c/11280/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@344 PS2, Line 344: "); table name http://gerrit.cloudera.org:8080/#/c/11280/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@619 PS2, Line 619: LOG.trace("loadFunctionNames({}): not cached yet", dbName); This reminds me of https://github.com/apache/impala/commit/352833b8cfcf5e0246f322fec1ee9b7612e0ed6a Mostafa once pointed out it can improve performance. http://gerrit.cloudera.org:8080/#/c/11280/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@647 PS2, Line 647: catalogServiceId_ = req.catalog_service_id; I think we should detect restarts here and log if (catalogServiceId_ ! = INITIAL_VERSION) LOG.warn(Detected a catalog restart...) http://gerrit.cloudera.org:8080/#/c/11280/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@688 PS2, Line 688: anyuthing nit: typo http://gerrit.cloudera.org:8080/#/c/11280/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@735 PS2, Line 735: debug debug is the default, you mean trace? http://gerrit.cloudera.org:8080/#/c/11280/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@747 PS2, Line 747: asMap().remove isn't invalidate() the right method? Are you using this for return val? http://gerrit.cloudera.org:8080/#/c/11280/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@896 PS2, Line 896: VersionedTableCacheKey Where is this used? It looks like loadTableNames() uses the base TableCacheKey and hence it is likely to be replaced in the cache. Maybe I'm missing something. -- 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: 2 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: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Sat, 25 Aug 2018 00:23:12 +0000 Gerrit-HasComments: Yes
