Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-4449: Revisit table locking pattern in the catalog ......................................................................
Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/5710/1//COMMIT_MSG Commit Message: PS1, Line 29: getAllCatalogObjects getCatalogObjects()? http://gerrit.cloudera.org:8080/#/c/5710/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: Line 926: while (true) { Regarding supportability, should we log a debug message/expose a average wait time metric via JMX if its stuck in these loops for longer than a configured value? IMO that makes it easier to debug queries hung at Catalog operations. (Same comment for other methods that follow this pattern, just wanted to hear you thoughts) Line 928: if (tbl.getLock().writeLock().tryLock()) { To avoid extra branch, may be we could refactor to something like catalogLock_.writeLock().lock(); if (!tbl.getLock().writeLock().tryLock()) { catalogLock_.writeLock().unlock(); continue; } . . . (Same comment for other places that uses this pattern) PS1, Line 950: continue; redundant? Line 1305: hdfsTable.setCatalogVersion(newCatalogVersion); - Just curious, is this an existing bug that we didn't set it if the partition exists? - May be we can move this line after L1285 before the try block since we are setting for both cases anyway and remove it at L1296? Line 1312: continue; redundant? http://gerrit.cloudera.org:8080/#/c/5710/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: Line 190: * } I think we should document the exception of getCatalogObjects() here. -- To view, visit http://gerrit.cloudera.org:8080/5710 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id08e21da31deb1f003b3cada4517651f3b3b2bb2 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-HasComments: Yes
