Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14038 )
Change subject: IMPALA-8823: DROP TABLE support for insert-only ACID tables ...................................................................... Patch Set 7: Code-Review+1 (5 comments) I am ok with the current solution, if we are sure that we want to do locking in impalad (and not catalogd). I added more details to a comment for the commit message. http://gerrit.cloudera.org:8080/#/c/14038/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14038/7//COMMIT_MSG@10 PS7, Line 10: Impala acquires an exclusive table lock in HMS It should be added that locking happens in Coordinator - the other candidate would be catalogd. Doing locking in catalogd / CatalogOpExecutor.dropTableOrView() would have some benefits: + locking / unlocking would be in the same class and function making the logic easier to understand + catalogd and HMS are often on the same node, which makes RPC-s cheaper + I guess that catalogd crashes less often, leading to less problems with leaked exclusive locks? I am not sure though - doing the locking there would mean holding the lock for less time, which seems a good thing, but acquiring the lock at a later time, which doesn't. So I am ok with the current solution, but this is not a self evident design decision, so it should be mentioned here. http://gerrit.cloudera.org:8080/#/c/14038/5/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/14038/5/fe/src/main/java/org/apache/impala/service/Frontend.java@510 PS5, Line 510: ble t > Thanks for catching this. 'table' could actually be FeIncompleteTable if An Can you add a comment about this? Being able to delete tables in problematic state totally makes sense, but it goes a bit against the "capability logic" that Impala should do only what it can surely do correctly. http://gerrit.cloudera.org:8080/#/c/14038/5/fe/src/main/java/org/apache/impala/service/Frontend.java@1721 PS5, Line 1721: y (MetaStoreClient client = metaStoreClientPool_.getClient()) { : transactionKeepalive_.deleteLock(lockId); > If there is a connectivity issue to HMS then we here end up with the lock r I think that it would make sense to try to clean up leaked locks faster then Hive's timeout, especially for exclusive locks, but I wouldn't do it in the current change. Created a Jira for this: IMPALA-8853 http://gerrit.cloudera.org:8080/#/c/14038/7/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/14038/7/fe/src/main/java/org/apache/impala/service/Frontend.java@1716 PS7, Line 1716: Releasess typo: Releases http://gerrit.cloudera.org:8080/#/c/14038/5/testdata/workloads/functional-query/queries/QueryTest/acid.test File testdata/workloads/functional-query/queries/QueryTest/acid.test: http://gerrit.cloudera.org:8080/#/c/14038/5/testdata/workloads/functional-query/queries/QueryTest/acid.test@79 PS5, Line 79: show tables; > Indeed, this is not needed anymore. invalidate metadata still remained here. -- To view, visit http://gerrit.cloudera.org:8080/14038 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34 Gerrit-Change-Number: 14038 Gerrit-PatchSet: 7 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Sat, 10 Aug 2019 18:16:45 +0000 Gerrit-HasComments: Yes
