Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/20516/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20516/2//COMMIT_MSG@16 PS2, Line 16: the in-flight : event list. > You could mention that the lock is on this object itself, and when it is lo Done http://gerrit.cloudera.org:8080/#/c/20516/2/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/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1054 PS2, Line 1054: // Not taking lock, rely on Db's internal locking. > Don't we need 'versionLock_.writeLock().unlock();' here? As we discussed offline, this is not needed. http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1064 PS2, Line 1064: == null > When it is not null, it is never empty, right? Should we have a preconditio tbh I don't know, I didn't want to change the existing logic in this regard. http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1065 PS2, Line 1065: // Not taking lock, rely on Table's internal locking. > Shouldn't we have 'versionLock_.writeLock().unlock();'? As we discussed offline, this is not needed. http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1076 PS2, Line 1076: : > nit: could you please create Jira for this? I used the current Jira and added part 1 to the commit message. http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1078 PS2, Line 1078: ot help > I don't think we need the word 'default' here Done http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1078 PS2, Line 1078: timeout > timeout Done http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1081 PS2, Line 1081: if (!tryWriteLock(tbl)) { : throw new CatalogException(String.format("Error during self-event evaluation " : + "for table %s due to lock contention", tbl.getFullName())); : } : versionLock_.writeLock().unlock(); > This, and the whole try-finally stmt could be moved to evaluateSelfEventFor I kept it there to minimize nesting in evaluateSelfEventForPartition. As that is a more complex function I prefer to keep the noise of try/catch outside. http://gerrit.cloudera.org:8080/#/c/20516/2/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/20516/2/fe/src/main/java/org/apache/impala/catalog/Db.java@102 PS2, Line 102: as a monit > nit: as a monitor object Done http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/Db.java@583 PS2, Line 583: ightEvents(lon > nit: thread safety Done http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/Table.java@152 PS2, Line 152: as a monit > nit: as a monitor object Done -- To view, visit http://gerrit.cloudera.org:8080/20516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd Gerrit-Change-Number: 20516 Gerrit-PatchSet: 3 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 28 Sep 2023 16:02:01 +0000 Gerrit-HasComments: Yes
