[Impala-ASF-CR] IMPALA-7469. Invalidate LocalCatalog cache based on topic updates

["Todd Lipcon (Code Review)" <ge...@cloudera.org>]

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 6:

(8 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
> ... all impalad coordinators ...
Yea... maybe the names are not so good. I didn't want to use "v1" and "v2" because they felt a little bit internal vs actually descriptive.

In theory, yea, the v2 impalad could use the "v1" catalog objects to perform invalidations. But, it's set to only subscribe to the v2 prefix, and making that dynamic seemed like more effort than it's worth. Any suggestions how to resolve this complication?


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,
turns out I have to remove this so that the fe tests work, since normally developers will be running in the default mode and the fe tests contact the running catalogd.

I think that's OK, though, since I already opened a JIRA so that the impalad will wait on startup until it gets a catalog update, even in 'v2' mode. So, if you misconfigure it, the impalad won't start since it won't get any catalog updates.


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.
Done


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
hmm, I could have sworn I changed this comment, gues si must have forgotten to commit or something. The limitation is actually all INVALIDATE METADATA, because it turns out the impalad also uses this to implement its *local* waiting to make INVALIDATE METADATA synchronous.. Will update comments.


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.
done


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?
Clarified


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
nice idea. I added that to this comment (still want to address separately since it's a rare race with not too bad ramifications)


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
yea, unfortunately updating a table comment would mean that we have to refetch everything about the table. The idea of making parittions immutable could avoid having to re-fetch all the partitions, at least.



-- 
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: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 01 Sep 2018 00:40:46 +0000
Gerrit-HasComments: Yes