Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14307 )
Change subject: IMPALA-7506: support global INVALIDATE METADATA in local catalog mode ...................................................................... Patch Set 6: (4 comments) Thanks Vihang's comments! > I looks like you are sending all the objects in the catalogd when you detect > the reset. Is it possible to not send the objects and just invalidate the > whole local catalog cache when you detect that the last reset version is > changed? Reseting coordinator's local catalog is ok. But coordinator needs to know when the reset() in catalogd finishs. For SYNC_DDL, it also needs to know the catalog topic version (not catalog version, it's the version of the statestore topic) so it can wait for other coordinators to be ready. I have an explanation with code links in "Why can’t we simply reset the cache of local catalog mode Coordinator for global INVALIDATE METADATA?" in the doc: https://docs.google.com/document/d/1-AoigzsQPgSGosW4vtVP8E7TiwoJJfLkAq3Rd6KvKBg http://gerrit.cloudera.org:8080/#/c/14307/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14307/5//COMMIT_MSG@41 PS5, Line 41: 1) No topic updates are sent from catalogd when the write lock of : versionLock is held in CatalogServiceCatalog.reset(). Note that the : update thread requires holding the read lock of versionLock. : 2) Authz changes before holding the write lock can only be sent in a : previous topic update or in the next topic update after reset(). : 3) No catalog objects are skipped in the topic update right after : reset(). See changes in GetCatalogDeltaContext > Did you consider the case when a reset is executed during execution of getC Oh, it's possible when reset() runs again after reset(), so getCatalogDelta thread is running with collectFullUpdates being set and will collect everything. In normal cases if getCatalogDelta() and reset() run concurrently, it's ok since getCatalogDelta runs with collectFullUpdates unset so will only collect updates in range of (fromVersion, toVersion]. After reset() holding the write lock, the updates are with version larger than toVersion. I'll update the proof here. It's ok for a previous update to contain some results of the reset(). The most important update, CATALOG type TCatalog object, can only get the correct lastResetCatalogVersion_ after reset() finishes. Before that, it just gets an old value of lastResetCatalogVersion_. (lastResetCatalogVersion_ is updated at the end of reset()) http://gerrit.cloudera.org:8080/#/c/14307/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/14307/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@721 PS5, Line 721: public long getCatalogDelta(long nativeCatalogServerPtr, long fromVersion) throws : TException { : long toVersion; : boolean collectFullUpdates; : versionLock_.readLock().lock(); : try { : toVersion = catalogVersion_; : > what happens if the reset thread takes a write lock after this code block? Then toVersion is small enough that won't cover updates after reset() acquiring the write lock. However, if reset() runs again after reset(), hasResetCatalog is true so all updates will be collected. As in the above comment, I think it's ok for a previous update to contain some results of a running reset() as long as it don't propagate the lastResetCatalogVersion_. BTW, with the changes in addTableToCatalogDeltaHelper(), in local catalog mode we no longer need to acquire the table locks. So collecting all table updates won't be blocked by concurrent DDLs. http://gerrit.cloudera.org:8080/#/c/14307/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1104 PS5, Line 1104: * which would also violate the semantics of SYNC_DDL. > Does the collectFullUpdates flag come in play in v1 as well? If yes, that s Yes, it should be used only in MINIMAL topic mode. I'll constrain this. http://gerrit.cloudera.org:8080/#/c/14307/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1601 PS5, Line 1601: sing for id just befo > It looks like the currentCatalogVersion may not be the right reset version Sorry, I should rename it to "lastResetCatalogVersion" or "lastResetBeginVersion" to avoid confusion. It's the same version that we return to the coordinator, meaning that all catalog objects with versions <= this will be invalidated. Coordinator first gets this version in the RPC response, then when receiving this again in the CATALOG type TCatalog object update (via statestore topic update), it knows that reset() has finished and all results are received. Propagating (catalogVersion_-1) is not we want since it's the current version - 1 after reset(). -- To view, visit http://gerrit.cloudera.org:8080/14307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib61a7ab1ffa062620ffbc2dadc34bd7a8ca9e549 Gerrit-Change-Number: 14307 Gerrit-PatchSet: 6 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Sat, 19 Oct 2019 02:02:58 +0000 Gerrit-HasComments: Yes
