Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/14038 )
Change subject: IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables ...................................................................... Patch Set 5: (5 comments) I think the code is in a good shape, only found nits. 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: long lock_id = -1; : if ((*request_state)->ShouldReleaseLock(lock_id)) { : Status st = exec_env_->frontend()->ReleaseLock(lock_id); : if (!st.ok()) VLOG_RPC << "Unable to release lock: " << lock_id; : } nit: Execute() is quite a high-level function. Should we put this code fragment into some Cleanup() funcion? 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: t typo 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@1792 PS5, Line 1792: * heartbeating the lock. nit: maybe you could extend the comment that is only for locks that don't have a transaction context. 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 anymore? Such tests are usually placed in test_hms_integration.py. http://gerrit.cloudera.org:8080/#/c/14038/5/testdata/workloads/functional-query/queries/QueryTest/acid.test@79 PS5, Line 79: invalidate metadata tt; Do we need invalidate metadata 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: 5 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Fri, 09 Aug 2019 15:46:02 +0000 Gerrit-HasComments: Yes
