Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/23382 )
Change subject: IMPALA-14400: Fix deadlock in CatalogServiceCatalog.getDbProperty() ...................................................................... Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/23382/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23382/4//COMMIT_MSG@28 PS4, Line 28: Remove acquireVersionReadLock() since there is only 1 callsite to it. nit: we might want to add it back in the future when adding more catalogTimelines, e.g. for event processing. http://gerrit.cloudera.org:8080/#/c/23382/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/23382/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@503 PS2, Line 503: > Accessing resetManager_ require olding writeLock() because there is a condi I see. It's still counter-intuitive that a reader method needs to acquire the write lock.. http://gerrit.cloudera.org:8080/#/c/23382/4/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/23382/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1501 PS4, Line 1501: if (forWrite_) { : // If we are holding the write lock, wait for any ongoing reset to complete : // before returning the Db. This ensures that writers always see the latest : // catalog state. I think we do have readers that also want to see the latest catalog state, e.g. from GetPartialCatalogObject requests. Previously we use waitInitialResetCompletion() to avoid returning a null Db. We can't avoid that now. http://gerrit.cloudera.org:8080/#/c/23382/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1505 PS4, Line 1505: resetManager_.waitOngoingMetadataFetch(dbName_); Previously we only do this when triggeredInitialReset_ is false. But it seems the performance impact is limited so I think it's OK. http://gerrit.cloudera.org:8080/#/c/23382/2/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: http://gerrit.cloudera.org:8080/#/c/23382/2/fe/src/main/java/org/apache/impala/service/JniCatalog.java@182 PS2, Line 182: catalogMetastoreServer_ = getCatalogMetastoreServer(catalogOpExecutor_); > Done Yeah, it's not enough. There are other places that could use getDb() before the initial reset, e.g. GetPartitalCatalogObject RPCs. -- To view, visit http://gerrit.cloudera.org:8080/23382 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3162472ea9531add77886bf1d0d73460ff34d07a Gerrit-Change-Number: 23382 Gerrit-PatchSet: 4 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Fri, 05 Sep 2025 13:12:50 +0000 Gerrit-HasComments: Yes
