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 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/15260/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15260/1//COMMIT_MSG@25 PS1, Line 25: After the fix I ran the test in a loop for 24 hrs (197 iterations) and > Can we reproduce the bug by keep altering the comment of a db? And even do Actually, I was able to reproduce the issue (even with this patch) on centos64 builds when I ran the test in a loop. I eventually, found that when we alter the db we only take a lock on metastoreDdlLock_ which cause this race condition. While taking a lock metastoreDdlLock_ works in CatalogOpExecutor it also unnecessarily blocks other DDLs (create/drop). I introduced a lock in Db similar to Table so that we can lock the Db for alter db operations. We may have to do some code refactor to reduce the duplicate locking logic, but I would like to do it in a separate change since it may modify several unrelated files. http://gerrit.cloudera.org:8080/#/c/15260/1/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/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@851 PS1, Line 851: ates if > nit: 4 spaces indent Done http://gerrit.cloudera.org:8080/#/c/15260/3/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/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@435 PS3, Line 435: database > Could you change this to "database object"? Done http://gerrit.cloudera.org:8080/#/c/15260/3/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/3/fe/src/main/java/org/apache/impala/catalog/Db.java@29 PS3, Line 29: import java.util.concurrent.locks.ReentrantLock; > nit: move it up into the "java.util" group Done http://gerrit.cloudera.org:8080/#/c/15260/3/fe/src/main/java/org/apache/impala/catalog/Db.java@529 PS3, Line 529: public void addToVersionsForInflightEvents(long versionNumber) { > Could you add a check that current thread is holding dbLock_ here? Good point, I added these checks on the db methods and also the remove method of the table which didn't have it originally. Also, found that one of the methods was not used and hence deleted it. http://gerrit.cloudera.org:8080/#/c/15260/3/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/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@211 PS3, Line 211: Db-level > "Db-level" shows like the whole db including all the underlying tables to m Done http://gerrit.cloudera.org:8080/#/c/15260/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1286 PS3, Line 1286: synchronized (metastoreDdlLock_) { > Should this be replaced with the db lock as well? yes, I think thats correct. Thanks for catching that. http://gerrit.cloudera.org:8080/#/c/15260/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2028 PS3, Line 2028: synchronized (metastoreDdlLock_) { > Same here. I think we should acquire the db lock instead. 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: 3 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Wed, 04 Mar 2020 18:59:03 +0000 Gerrit-HasComments: Yes
