Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/16549 )
Change subject: IMPALA-6671: Skip locked tables from topic updates ...................................................................... Patch Set 10: (19 comments) http://gerrit.cloudera.org:8080/#/c/16549/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16549/10//COMMIT_MSG@46 PS10, Line 46: 2 nit: 3 http://gerrit.cloudera.org:8080/#/c/16549/10/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/16549/10/be/src/catalog/catalog-server.cc@79 PS10, Line 79: the nit: redundant "the" http://gerrit.cloudera.org:8080/#/c/16549/10/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/16549/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@25 PS10, Line 25: import java.sql.Time; nit: unused import http://gerrit.cloudera.org:8080/#/c/16549/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@40 PS10, Line 40: import java.util.concurrent.locks.ReentrantLock; nit: unused import http://gerrit.cloudera.org:8080/#/c/16549/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@212 PS10, Line 212: maxSkippedUpdatesLockContention nit: maxSkippedUpdatesLockContention_ http://gerrit.cloudera.org:8080/#/c/16549/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@214 PS10, Line 214: topicUpdateTblLockMaxWaitTimeMs nit: topicUpdateTblLockMaxWaitTimeMs_ http://gerrit.cloudera.org:8080/#/c/16549/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@453 PS10, Line 453: lock It may be useful to know the lock type (read/write) in debugging. http://gerrit.cloudera.org:8080/#/c/16549/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@460 PS10, Line 460: 0, TimeUnit.SECONDS Can we directly use the specified timeout here? I think the answer is no and the reason is we want to release the catalog versionLock immediately. If so, I think it's worth a comment here. http://gerrit.cloudera.org:8080/#/c/16549/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1395 PS10, Line 1395: topicUpdateTblLockMaxWaitTimeMs I think the timeout should be topicUpdateTblLockMaxWaitTimeMs / maxAttempts http://gerrit.cloudera.org:8080/#/c/16549/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1399 PS10, Line 1399: break; nit: we can return true directly here. Then we don't need the lockAcquired var. http://gerrit.cloudera.org:8080/#/c/16549/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1416 PS10, Line 1416: // if pendingVersionUpdated is false it means that tblVersion has been changed : // and hence we didn't update the pendingVersion. We retry once to acquire a read : // lock. Should we update tblVersion to be hdfsTable.getCatalogVersion() in this case? http://gerrit.cloudera.org:8080/#/c/16549/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2215 PS10, Line 2215: updatedTbl.setCatalogVersion(incrementAndGetCatalogVersion()); If the existingTbl has a pendingVersion and both instances are HdfsTable, we loss the pendingVersion in the updatedTbl. But looks like it's ok because incrementAndGetCatalogVersion() will always get a version larger than the pendingVersion. This is just a corner case that only happens when a table loading is triggered due to stale writeIdList. It's worth a comment here. BTW, what if we use incrementAndGetCatalogVersion() at the end of table modifications to set its catalog version? Is it a smipler solution than the pendingVersion solution? We just need to avoid deadlocks since incrementAndGetCatalogVersion() requires the catalog versionLock but we are holding the table lock in table modifications. It seems ok if we always use tryLock to acquire the table lock and give up the catalog version lock when it fails. http://gerrit.cloudera.org:8080/#/c/16549/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3383 PS10, Line 3383: // TODO(todd): consider a read-write lock here. We can remove this TODO now. http://gerrit.cloudera.org:8080/#/c/16549/10/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/16549/10/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@299 PS10, Line 299: Hdfs nit: HdfsTable? http://gerrit.cloudera.org:8080/#/c/16549/10/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2709 PS10, Line 2709: then nit: the http://gerrit.cloudera.org:8080/#/c/16549/10/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/16549/10/fe/src/main/java/org/apache/impala/catalog/Table.java@28 PS10, Line 28: nit: blank line http://gerrit.cloudera.org:8080/#/c/16549/10/fe/src/main/java/org/apache/impala/catalog/Table.java@29 PS10, Line 29: import java.util.concurrent.locks.ReentrantLock; nit: unused import http://gerrit.cloudera.org:8080/#/c/16549/10/fe/src/main/java/org/apache/impala/catalog/Table.java@85 PS10, Line 85: true nit: it is worth mentioning that this means using a fair ordering policy. http://gerrit.cloudera.org:8080/#/c/16549/10/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/16549/10/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@5088 PS10, Line 5088: nit: of -- To view, visit http://gerrit.cloudera.org:8080/16549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic657b96edbcdc94c6b906e7ca59291f4e4715655 Gerrit-Change-Number: 16549 Gerrit-PatchSet: 10 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Shant Hovsepian <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Wed, 25 Nov 2020 09:15:53 +0000 Gerrit-HasComments: Yes
