Vihang Karajgaonkar 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) 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? 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 getCatalogDelta? Looks like it is possible for this to happen since the getCatalogDelta takes a read lock and releases in the for loop. So the reset thread can take a write lock in between the getCatalogDelta which effectively means that the topic may have some state before reset and some state after the 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? 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. > I see what you are saying, nice find. Agree that we shouldn't take any lock Does the collectFullUpdates flag come in play in v1 as well? If yes, that seems unnecessary. 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 since once we assign currentCatalogVersion and the place where we take the write lock is not atomic. May be you should directly use (catalogVersion_-1) here since you already hold the write lock -- 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: Fri, 18 Oct 2019 17:00:05 +0000 Gerrit-HasComments: Yes
