You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Gabor Kaszab (Code Review)" <ge...@cloudera.org> on 2019/08/08 22:01:31 UTC
[Impala-ASF-CR] IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Gabor Kaszab has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14038
Change subject: IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
......................................................................
IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M common/thrift/Frontend.thrift
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M testdata/workloads/functional-query/queries/QueryTest/acid.test
13 files changed, 146 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/14038/1
--
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: newchange
Gerrit-Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
Gerrit-Change-Number: 14038
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Vihang Karajgaonkar, Zoltan Borok-Nagy, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14038
to look at the new patch set (#9).
Change subject: IMPALA-8823: DROP TABLE support for insert-only ACID tables
......................................................................
IMPALA-8823: DROP TABLE support for insert-only ACID tables
Enhances Impala to be able to drop insert-only transactional tables.
In order to do this Impala acquires an exclusive table lock in HMS
before performing the drop operation and releases the lock once
dropping the table finished.
Previously locking and heartbeating was done on coordinator side. For
DROP TABLE all of these are done from Catalog side. This means that
alongside Impala coordinators now Catalog also does heartbeating
towards HMS.
Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
---
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
R fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M testdata/workloads/functional-query/queries/QueryTest/acid.test
M tests/metadata/test_hms_integration.py
M tests/query_test/test_acid.py
11 files changed, 207 insertions(+), 40 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/14038/9
--
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: newpatchset
Gerrit-Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
Gerrit-Change-Number: 14038
Gerrit-PatchSet: 9
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Vihang Karajgaonkar, Zoltan Borok-Nagy, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14038
to look at the new patch set (#10).
Change subject: IMPALA-8823: DROP TABLE support for insert-only ACID tables
......................................................................
IMPALA-8823: DROP TABLE support for insert-only ACID tables
Enhances Impala to be able to drop insert-only transactional tables.
In order to do this Impala acquires an exclusive table lock in HMS
before performing the drop operation and releases the lock once
dropping the table finished.
INSERT statement does the locking and heartbeating on coordinator
side but for DROP TABLE all of these are done from Catalog side. This
means that alongside Impala coordinators now Catalog also does
heartbeating towards HMS.
Testing:
- E2E test: Dropped a table, re-created it and dropped again to
check if no locks remained in HMS.
- E2E test: After dropping a table from Impala checked if Hive also
sees it being dropped.
- Manual test: With a hacked Impala that runs a drop table long
enough I checked that there is a table lock entry in HMS during the
execution and disappears once the query finishes.
Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
---
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
R fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M testdata/workloads/functional-query/queries/QueryTest/acid.test
M tests/metadata/test_hms_integration.py
M tests/query_test/test_acid.py
11 files changed, 206 insertions(+), 40 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/14038/10
--
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: newpatchset
Gerrit-Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
Gerrit-Change-Number: 14038
Gerrit-PatchSet: 10
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 8:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/4231/ : Initial code review checks failed. See linked job for details on the failure.
--
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: 8
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Aug 2019 09:24:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14038/1/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/1/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@542
PS1, Line 542: * @param txnId The transaction ID associated with the lock. Zero if the lock doesn't belong to a
line too long (99 > 90)
--
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: 1
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Aug 2019 22:02:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
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 <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Aug 2019 13:38:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 3:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/4192/ : Initial code review checks failed. See linked job for details on the failure.
--
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: 3
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Aug 2019 22:28:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14038/2/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/2/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@542
PS2, Line 542: * @param txnId The transaction ID associated with the lock. Zero if the lock doesn't belong to a
line too long (99 > 90)
--
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: 2
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Aug 2019 22:05:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/4190/ : Initial code review checks failed. See linked job for details on the failure.
--
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: 1
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Aug 2019 22:42:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 12:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4780/ DRY_RUN=false
--
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: 12
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Aug 2019 13:58:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14038
to look at the new patch set (#3).
Change subject: IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
......................................................................
IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M common/thrift/Frontend.thrift
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M testdata/workloads/functional-query/queries/QueryTest/acid.test
13 files changed, 146 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/14038/3
--
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: newpatchset
Gerrit-Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
Gerrit-Change-Number: 14038
Gerrit-PatchSet: 3
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14038 )
Change subject: IMPALA-8823: DROP TABLE support for insert-only ACID tables
......................................................................
IMPALA-8823: DROP TABLE support for insert-only ACID tables
Enhances Impala to be able to drop insert-only transactional tables.
In order to do this Impala acquires an exclusive table lock in HMS
before performing the drop operation and releases the lock once
dropping the table finished.
INSERT statement does the locking and heartbeating on coordinator
side but for DROP TABLE all of these are done from Catalog side. This
means that alongside Impala coordinators now Catalog also does
heartbeating towards HMS.
Testing:
- E2E test: Dropped a table, re-created it and dropped again to
check if no locks remained in HMS.
- E2E test: After dropping a table from Impala checked if Hive also
sees it being dropped.
- Manual test: With a hacked Impala that runs a drop table long
enough I checked that there is a table lock entry in HMS during the
execution and disappears once the query finishes.
Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
Reviewed-on: http://gerrit.cloudera.org:8080/14038
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
R fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M testdata/workloads/functional-query/queries/QueryTest/acid.test
M tests/metadata/test_hms_integration.py
M tests/query_test/test_acid.py
11 files changed, 205 insertions(+), 40 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
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: merged
Gerrit-Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
Gerrit-Change-Number: 14038
Gerrit-PatchSet: 13
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sat, 10 Aug 2019 18:16:45 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4236/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Aug 2019 14:18:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 2:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/4191/ : Initial code review checks failed. See linked job for details on the failure.
--
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: 2
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Aug 2019 22:45:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Aug 2019 12:56:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 12: Verified+1
--
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: 12
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Aug 2019 18:07:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Vihang Karajgaonkar, Zoltan Borok-Nagy, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14038
to look at the new patch set (#6).
Change subject: IMPALA-8823: DROP TABLE support for insert-only ACID tables
......................................................................
IMPALA-8823: DROP TABLE support for insert-only ACID tables
Enhances Impala to be able to drop insert-only transactional tables.
In order to do this Impala acquires an exclusive table lock in HMS
before performing the drop operation and releases the lock once
dropping the table finished.
Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/thrift/Frontend.thrift
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M testdata/workloads/functional-query/queries/QueryTest/acid.test
M tests/metadata/test_hms_integration.py
M tests/query_test/test_acid.py
16 files changed, 185 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/14038/6
--
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: newpatchset
Gerrit-Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
Gerrit-Change-Number: 14038
Gerrit-PatchSet: 6
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Vihang Karajgaonkar, Zoltan Borok-Nagy, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14038
to look at the new patch set (#8).
Change subject: IMPALA-8823: DROP TABLE support for insert-only ACID tables
......................................................................
IMPALA-8823: DROP TABLE support for insert-only ACID tables
Enhances Impala to be able to drop insert-only transactional tables.
In order to do this Impala acquires an exclusive table lock in HMS
before performing the drop operation and releases the lock once
dropping the table finished.
Previously locking and heartbeating was done on coordinator side. For
DROP TABLE all of these are done from Catalog side. This means that
alongside Impala coordinators now Catalog also does heartbeating
towards HMS.
Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
---
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
R fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M testdata/workloads/functional-query/queries/QueryTest/acid.test
M tests/metadata/test_hms_integration.py
M tests/query_test/test_acid.py
11 files changed, 206 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/14038/8
--
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: newpatchset
Gerrit-Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
Gerrit-Change-Number: 14038
Gerrit-PatchSet: 8
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 4:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/4202/ : Initial code review checks failed. See linked job for details on the failure.
--
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: 4
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 09 Aug 2019 10:21:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar 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:
(3 comments)
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
Can this table be instanceof Incomplete table in case there is a tableloading exception during analysis?
http://gerrit.cloudera.org:8080/#/c/14038/5/fe/src/main/java/org/apache/impala/service/Frontend.java@1721
PS5, Line 1721: transactionKeepalive_.deleteLock(lockId);
: MetastoreShim.releaseLock(client.getHiveClient(), lockId);
Is it possible that we are silently leaking locks if for instance MetastoreShim.releaseLock throws an exception (say due to connection issue with HMS)?
According to me both these statements should happen atomically. Either they both succeed or not.
http://gerrit.cloudera.org:8080/#/c/14038/5/fe/src/main/java/org/apache/impala/service/Frontend.java@1809
PS5, Line 1809: lockId = MetastoreShim.acquireLock(client.getHiveClient(), 0L, lockComponents,
: LOCK_RETRIES, LOCK_RETRY_WAIT_SECONDS);
: transactionKeepalive_.addLock(lockId, queryCtx);
same comment as previous regarding the atomicity of these two statemnts. May be provide this as a API in the transactionKeepAlive so that we can reuse that code elsewhere if needed.
--
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 <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 09 Aug 2019 21:14:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 10: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14038/10/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/10/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1577
PS10, Line 1577: Table tbl = catalog_.getTableIfCachedNoThrow(tableName.getDb(),
: tableName.getTbl());
nit: fits to one line
--
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 <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Aug 2019 13:54:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14038
to look at the new patch set (#5).
Change subject: IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
......................................................................
IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M common/thrift/Frontend.thrift
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M testdata/workloads/functional-query/queries/QueryTest/acid.test
M tests/query_test/test_acid.py
14 files changed, 157 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/14038/5
--
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: newpatchset
Gerrit-Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
Gerrit-Change-Number: 14038
Gerrit-PatchSet: 5
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 09 Aug 2019 15:46:02 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 12: Code-Review+2
--
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: 12
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Aug 2019 13:58:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
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 8:
(5 comments)
Re-wrote the code to do the locking and heartbeating from catalogd instead. This also cleaned up the code a little bit.
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 candidat
I decided to do the locking from catalogd instead, as you suggested. Thanks for the explanation.
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
> Can you add a comment about this? Being able to delete tables in problemati
I'm not sure what comment you expect here.
http://gerrit.cloudera.org:8080/#/c/14038/5/fe/src/main/java/org/apache/impala/service/Frontend.java@1721
PS5, Line 1721: long txnId = queryCtx.getTransaction_id();
: return MetastoreShim.allocateTableWriteId(hmsClient, txnId
> I think that it would make sense to try to clean up leaked locks faster the
Thanks for opening a Jira for this!
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: f (!AcidU
> typo: Releases
Done
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: ---- RESULTS
> invalidate metadata still remained here.
Done
--
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: 8
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Aug 2019 08:59:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
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 11: Code-Review+2
carry +2 from Csaba. Thanks all for the quick turnarounds!
--
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: 11
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Aug 2019 13:58:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
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 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14038/10/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/10/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1577
PS10, Line 1577: Table tbl = catalog_.getTableIfCachedNoThrow(tableName.getDb(), tableName.getTbl());
: long lockId = -1;
> nit: fits to one line
Done
--
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: 11
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Aug 2019 13:57:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14038
to look at the new patch set (#2).
Change subject: IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
......................................................................
IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M common/thrift/Frontend.thrift
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M testdata/workloads/functional-query/queries/QueryTest/acid.test
13 files changed, 146 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/14038/2
--
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: newpatchset
Gerrit-Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
Gerrit-Change-Number: 14038
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14038/6/tests/metadata/test_hms_integration.py
File tests/metadata/test_hms_integration.py:
http://gerrit.cloudera.org:8080/#/c/14038/6/tests/metadata/test_hms_integration.py@742
PS6, Line 742: ;
flake8: E703 statement ends with a semicolon
--
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 <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sat, 10 Aug 2019 13:04:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 9:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4233/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 9
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Aug 2019 10:10:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Vihang Karajgaonkar, Zoltan Borok-Nagy, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14038
to look at the new patch set (#11).
Change subject: IMPALA-8823: DROP TABLE support for insert-only ACID tables
......................................................................
IMPALA-8823: DROP TABLE support for insert-only ACID tables
Enhances Impala to be able to drop insert-only transactional tables.
In order to do this Impala acquires an exclusive table lock in HMS
before performing the drop operation and releases the lock once
dropping the table finished.
INSERT statement does the locking and heartbeating on coordinator
side but for DROP TABLE all of these are done from Catalog side. This
means that alongside Impala coordinators now Catalog also does
heartbeating towards HMS.
Testing:
- E2E test: Dropped a table, re-created it and dropped again to
check if no locks remained in HMS.
- E2E test: After dropping a table from Impala checked if Hive also
sees it being dropped.
- Manual test: With a hacked Impala that runs a drop table long
enough I checked that there is a table lock entry in HMS during the
execution and disappears once the query finishes.
Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
---
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
R fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M testdata/workloads/functional-query/queries/QueryTest/acid.test
M tests/metadata/test_hms_integration.py
M tests/query_test/test_acid.py
11 files changed, 205 insertions(+), 40 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/14038/11
--
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: newpatchset
Gerrit-Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
Gerrit-Change-Number: 14038
Gerrit-PatchSet: 11
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Vihang Karajgaonkar, Zoltan Borok-Nagy, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14038
to look at the new patch set (#7).
Change subject: IMPALA-8823: DROP TABLE support for insert-only ACID tables
......................................................................
IMPALA-8823: DROP TABLE support for insert-only ACID tables
Enhances Impala to be able to drop insert-only transactional tables.
In order to do this Impala acquires an exclusive table lock in HMS
before performing the drop operation and releases the lock once
dropping the table finished.
Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/thrift/Frontend.thrift
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M testdata/workloads/functional-query/queries/QueryTest/acid.test
M tests/metadata/test_hms_integration.py
M tests/query_test/test_acid.py
16 files changed, 185 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/14038/7
--
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: newpatchset
Gerrit-Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
Gerrit-Change-Number: 14038
Gerrit-PatchSet: 7
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4203/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 09 Aug 2019 12:34:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4213/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sat, 10 Aug 2019 13:44:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy 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 9: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14038/9//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14038/9//COMMIT_MSG@17
PS9, Line 17:
nit: could you add a paragraph about testing?
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@604
PS9, Line 604: LOG.info("Releasing lock: " + lockId);
nit: I think we should log both acquire and release, or don't log any of them.
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: If the heartbeating is initialized from the
: // coordinator then queryCtx is populated if it is initialized from the catalogd there
: // is no TQueryCtx available so a cause string is populated.
nit: I'd rather explain what it does than how it is currently used.
--
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: 9
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Aug 2019 12:41:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4214/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sat, 10 Aug 2019 13:53:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
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 <ga...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sat, 10 Aug 2019 13:04:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14038
to look at the new patch set (#4).
Change subject: IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
......................................................................
IMPALA-8823 WIP: DROP TABLE support for insert-only ACID tables
Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M common/thrift/Frontend.thrift
M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M testdata/workloads/functional-query/queries/QueryTest/acid.test
13 files changed, 152 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/14038/4
--
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: newpatchset
Gerrit-Change-Id: Ic41ca73268c4b75af5a08fe3dd1ada1df3f6fd34
Gerrit-Change-Number: 14038
Gerrit-PatchSet: 4
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-8823: DROP TABLE support for insert-only ACID tables
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4237/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 11
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Aug 2019 14:35:00 +0000
Gerrit-HasComments: No