Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check ...................................................................... Patch Set 2: (11 comments) Nice change! 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 locked no other locks are taken, i.e. we cannot deadlock. 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? 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 precondition check for this after this if stmt? 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();'? 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? http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1078 PS2, Line 1078: time out timeout http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1078 PS2, Line 1078: default I don't think we need the word 'default' here 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 evaluateSelfEventForPartition() 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 monitor nit: as a monitor object http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/Db.java@583 PS2, Line 583: thready safity nit: thread safety 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 monitor nit: as a monitor object -- 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: 2 Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 28 Sep 2023 15:18:28 +0000 Gerrit-HasComments: Yes
