Csaba Ringhofer 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) 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 level of logging for event turn out to be not self events? 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 you remove that too? 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 table's lock should be held. 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 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@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, while at others they are called before it. For me it seems more logical to call them outside, as the "db.getLock().unlock();" in finally makes only sense for the case when these functions succeed. http://gerrit.cloudera.org:8080/#/c/15260/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4698 PS7, Line 4698: void optional: this could return long and the " // Get a new catalog version to assign to the database being altered. long newCatalogVersion = catalog_.incrementAndGetCatalogVersion(); catalog_.getLock().writeLock().unlock(); " block could be move to it as the function is always called together with these operations. -- 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 14:22:04 +0000 Gerrit-HasComments: Yes
