Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15260 )
Change subject: IMPALA-9357: Fix race condition in alter_database event ...................................................................... Patch Set 7: (6 comments) > (1 comment) > > Have another look for deadlocks. I think it's ok since we always > keep the lock acquiring order: > > catalog version lock -> db object lock -> functions lock > catalog version lock -> table lock > metastoreDdlLock_ -> functions lock (in RefreshFunctions) > > Also, there are no operations requiring both db-object lock and > table lock. However, it'd be better to add test coverage for > concurrent ddls (especially db related ddls) in > tests/custom_cluster/test_concurrent_ddls.py Thanks for checking again. I added some more ddls which are specific to dbs in the test_concurrent_ddls.py. http://gerrit.cloudera.org:8080/#/c/15260/7/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/15260/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@854 PS7, Line 854: info > The same log line is only debug in case of tables. What is the intended lev Thanks for spotting that. I think we should log at info level is the event is not determined as self-event since that would cause a table or partition level refresh by the event processor. Changed the partition level logs to info as well. If this turns out to be too verbose I think we can revisit this later but for now I think its helpful for debugging. http://gerrit.cloudera.org:8080/#/c/15260/7/fe/src/main/java/org/apache/impala/catalog/Db.java File fe/src/main/java/org/apache/impala/catalog/Db.java: http://gerrit.cloudera.org:8080/#/c/15260/7/fe/src/main/java/org/apache/impala/catalog/Db.java@506 PS7, Line 506: * Removes a given version from the collection of version numbers for in-flight events > HdfsPartition also has an unused getVersionsForInflightEvents function. Can Done http://gerrit.cloudera.org:8080/#/c/15260/7/fe/src/main/java/org/apache/impala/catalog/Db.java@511 PS7, Line 511: Preconditions.checkState(dbLock_.isHeldByCurrentThread(), > Can you add a similar check to HDFSPartitions? I think that it's parant tab Done http://gerrit.cloudera.org:8080/#/c/15260/7/fe/src/main/java/org/apache/impala/catalog/Db.java@524 PS7, Line 524: public void addToVersionsForInflightEvents(long versionNumber) { > same as at line 511 Done http://gerrit.cloudera.org:8080/#/c/15260/7/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/15260/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@244 PS7, Line 244: * Operations that CREATE/DROP catalog objects such as tables and databases employ the > We should mention that CREATE/DROP functions are now not following this rul Updated the doc. http://gerrit.cloudera.org:8080/#/c/15260/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1292 PS7, Line 1292: tryLock(db, "creating function " + fn.getClass().getSimpleName()); : // Get a new catalog version to assign to the database being altered. This is : // needed for events processor as this method creates alter database events. : long newCatalogVersion = catalog_.incrementAndGetCatalogVersion(); : catalog_.getLock().writeLock().unlock(); > consistency: in some call sites these functions are called in the try block makes sense. Done. -- To view, visit http://gerrit.cloudera.org:8080/15260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I472fd8a55740769ee5cdb84e48422a4ab39a8d1e Gerrit-Change-Number: 15260 Gerrit-PatchSet: 7 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Mon, 09 Mar 2020 19:13:08 +0000 Gerrit-HasComments: Yes
