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 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/14038/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14038/9//COMMIT_MSG@13 PS9, Line 13: I think that it would be better to be more specific and write that INSERT did it on the coordinator side. The reason for this is that INSERT is a DML statement that starts work on the coordinator + executors and touches the catalogd only after the executer finished. Creating the transaction in the catalogd at the start would have made an extra coordinator->catalogd RPC necessary. For DDL statements the logic above does not apply. because DDL logic is mainly done in the catalogd, the coordinator acts more or less as a proxy towards the client. http://gerrit.cloudera.org:8080/#/c/14038/9/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/14038/9/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@585 PS9, Line 585: * @param lockId is the lock ID to be released. : * @throws TransactionException It would be better to log the exception's message too. Moving logging to releaseLock() and change it to return bool instead of exceptions could make the caller logic simpler. http://gerrit.cloudera.org:8080/#/c/14038/9/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/14038/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1559 PS9, Line 1559: anular checks to provid According to line 1588-1590 it is possible that the table is not loaded yet, mainly in case of LocalCatalog configuration. I think that the logic in line 1582 - 1598 could be moved above this statement. http://gerrit.cloudera.org:8080/#/c/14038/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1573 PS9, Line 1573: This can throw and exception which would lead to an error returned to the client while the table was actually successfully dropped. See my comment at MetastoreShim.java:585 for a possible solution. http://gerrit.cloudera.org:8080/#/c/14038/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1580 PS9, Line 1580: mmary(resp, dbNotExist) +1 for creating this function 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: table > I'm not sure what comment you expect here. I expected something like "FeIncompleteTable means that the table could not be loaded successfully. Still try do delete the table to be able delete tables with corrupted metadata." In CatalogOpExecuter there is some explanation about this, so I am ok with adding comments at this logic's new place. -- 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: 5 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: Tue, 13 Aug 2019 12:56:03 +0000 Gerrit-HasComments: Yes
