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 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/14038/5/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/14038/5/be/src/service/impala-server.cc@1070 PS5, Line 1070: ReleaseLock(request_state); : if (!status.ok() && registered_request_state) { : discard_result(UnregisterQuery((*request_state)->query_id(), false, &status)); : } : r > nit: Execute() is quite a high-level function. Should we put this code frag I've put it into ReleaseLock() function. Is it ok? http://gerrit.cloudera.org:8080/#/c/14038/5/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/14038/5/common/thrift/Frontend.thrift@531 PS5, Line 531: > typo Done 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 > Can this table be instanceof Incomplete table in case there is a tableloadi Thanks for catching this. 'table' could actually be FeIncompleteTable if Analyzer.getTable() throws called from DropTableOrViewStmt.analyze(). 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); > Is it possible that we are silently leaking locks if for instance Metastore If there is a connectivity issue to HMS then we here end up with the lock remaining in HMS but the heartbeating stopped from Impala. Due to the lack of heartbeats HMS will clear out the lock in a configured threshold time. So I think this is sufficient guarantee here that even if releaseLock() throws eventually the lock is released. http://gerrit.cloudera.org:8080/#/c/14038/5/fe/src/main/java/org/apache/impala/service/Frontend.java@1792 PS5, Line 1792: * Creates an exclusive lock for a particular table and acquires it in the HMS. Starts > nit: maybe you could extend the comment that is only for locks that don't h Done http://gerrit.cloudera.org:8080/#/c/14038/5/fe/src/main/java/org/apache/impala/service/Frontend.java@1809 PS5, Line 1809: ng lockId = -1L; : try (MetaStoreClient client = metaStoreClientPool_.getClient()) { : lockId = MetastoreShim.acquireLock(client.getHiv > same comment as previous regarding the atomicity of these two statemnts. Ma Here, atomicity is not desired in my opinion as we can't reach a state where one of these succeeded but the other failed. If acquireLock() throws then addLock() is not executed. If acquireLock() succeeds then there is no use-case where addLock fails as it simply adds some values into a Map. 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@3 PS5, Line 3: # Create a table with Hive and run insert, select, and drop from Impala on it. > Can we add an interop test that checks that Hive doesn't see the table anym Done http://gerrit.cloudera.org:8080/#/c/14038/5/testdata/workloads/functional-query/queries/QueryTest/acid.test@79 PS5, Line 79: show tables; > Do we need invalidate metadata here? Indeed, this is not needed anymore. -- 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: 6 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Sat, 10 Aug 2019 13:04:56 +0000 Gerrit-HasComments: Yes
