Gabor Kaszab 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 10: (9 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: INSERT sta > I think that it would be better to be more specific and write that INSERT d Done http://gerrit.cloudera.org:8080/#/c/14038/9//COMMIT_MSG@17 PS9, Line 17: > nit: could you add a paragraph about testing? Done 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: LOG.error("Failed to release lock as a cleanup step after acquiring a lock " + : "has failed: " + lockId + > It would be better to log the exception's message too. See my comment in CatalogOpExecutor about returning bool instead of throwing. I can add the exception text to the log msg here. http://gerrit.cloudera.org:8080/#/c/14038/9/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@604 PS9, Line 604: try { > nit: I think we should log both acquire and release, or don't log any of th Dropped logging in releaseLock() as I'm a bit afraid that we might flood logs if we log both in releaseLock() and acquireLock(). http://gerrit.cloudera.org:8080/#/c/14038/9/fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java File fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java: http://gerrit.cloudera.org:8080/#/c/14038/9/fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java@57 PS9, Line 57: Stores either a TQueryCtx or a cause : // string. toString() returns the stored TQueryCtx if it is set or the string cause : // otherwise. > nit: I'd rather explain what it does than how it is currently used. Done 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: s, ensure that it is lo > According to line 1588-1590 it is possible that the table is not loaded yet Done http://gerrit.cloudera.org:8080/#/c/14038/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1573 PS9, Line 1573: he above was jus > This can throw and exception which would lead to an error returned to the c I think we should raise an error if releasing a lock failed even after successfully dropping the table. Otherwise we would swallow it and receive error the next time we recreate the table and try to touch it. E.g.: 1) drop table if exists tbl_name; 2) create table tbl_name ... 3) Insert into tbl_name Here 3 would complain about not being able to acquire a lock even though 2) should have already told us that it couldn't release it's lock. http://gerrit.cloudera.org:8080/#/c/14038/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1580 PS9, Line 1580: null && !(tbl instanceo > +1 for creating this function Thx :) 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: setTr > I expected something like "FeIncompleteTable means that the table could not That is already commented in DropTableOrViewStmt as far as I know why we allow the drop table not to exit when table loading fails. It's unrelated to locking so nothing to comment here in my opinion. -- 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: 10 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 13:38:38 +0000 Gerrit-HasComments: Yes
