You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org> on 2019/07/31 18:42:35 UTC
[Impala-ASF-CR] WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
Zoltan Borok-Nagy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13968
Change subject: WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
Initial prototype for locking tables and heartbeating the transaction
and the lock.
Currently only works for INSERT statement, and only locks the target
table.
New TransactionKeepalive class starts a daemon thread that
periodically heartbeats the registered transactions and locks.
TODO in following PSs:
* lock all tables of INSERT
* add some tests
* put methods into MetaStoreShim for compatibility
Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
---
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
6 files changed, 177 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/13968/1
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 3:
(17 comments)
Thanks everyone for the comments!
http://gerrit.cloudera.org:8080/#/c/13968/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13968/2//COMMIT_MSG@19
PS2, Line 19: * add tests
> Metrics for the following would be good (just some ideas)
Added logs for observability but I agree that such metrics would be very useful.
Is it OK if I add them in a follow-up Jira later? It would need some further work plus investigation of how to JNI call from the Frontend to some backend class object, e.g. the coordinator.
http://gerrit.cloudera.org:8080/#/c/13968/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/13968/2/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@516
PS2, Line 516: long txnId, long lockId) throws TransactionException {
> Should we log these? I also asked this in the caller, maybe this is a bette
Added LOG.info() for the exception handlers.
http://gerrit.cloudera.org:8080/#/c/13968/2/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/13968/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1683
PS2, Line 1683: }
> Do we have commitTransaction calls somewhere that actively call the transac
Currently commit is done by Catalogd so we cannot call deleteTransaction() immediately.
But after commit is done by Catalogd the coordinator invokes unregisterTransaction() via JNI that calls deleteTransaction().
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1718
PS2, Line 1718: /**
> Can you give tables.size() to the arraylist constructor?
Done
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1726
PS2, Line 1726: private void createLockForInsert(Long txnId, Collection<FeTable> tables,
> Does hive have code to handle deadlock prevention?
Since we put all of our lockComponents (table locks) into a single LockRequest and give it a transaction context as well it shouldn't deadlock.
https://github.com/apache/hive/blob/d7475aa98f6a2fc813e2e1c0ad99f902cb28cc00/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java#L2240
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
File fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java:
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@44
PS2, Line 44: // TODO: should be calculated from hive.txn.timeout or metastore.txn.timeout
> Never used?
Removed it.
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@48
PS2, Line 48:
> maybe for a later commit:
Added a simple check for start. I save the creation time of the transaction or lock then only heartbeat them if they are olden than the sleep interval.
Added TODO about it to be more clever later.
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@54
PS2, Line 54: private Map<Long, Long> transactions_ = new HashMap<>();
> I think it would be worth logging some information each time this thread wa
I log how much open transactions or independent locks are open. And only log when there's any to heartbeat.
Also added logging about how much time heartbeating took.
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@58
PS2, Line 58:
> as Tim noticed, this is never true, right?
I removed it. Currently TransactionKeepalive thread runs forever.
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@67
PS2, Line 67: 's deepcopy the transactions and locks
> I checked what Hive does, and it only sends heartbeats for the transaction
Thanks, it wasn't clear to me from the docs and the RPC didn't throw an exception when I heartbeated both.
With than in mind I had to do a bunch of changes, but I believe it'll be beneficial long-term, so thanks again.
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@68
PS2, Line 68: Map<Long, Long> copyOfTransactions;
> I'm curious if the HMS supports or plans to support a bulk heartbeat API wh
There is also a heartbeatTxnRange() method but the docs say "it's for streaming ingest, everyone else should use heartbeat(txnId, lockId)"
https://github.com/apache/hive/blob/d7475aa98f6a2fc813e2e1c0ad99f902cb28cc00/standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java#L3161
But for a single transaction there is a single lock, plus we only need to heartbeat the transaction with the dummy lock id (0) as Csaba found out.
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@73
PS2, Line 73: copyOfLocks = locks_.entrySet().stream().collect(
> Doing this way makes me a bit worried about multiple coordinators starting
I added the logic requested from Tim. I think the coordinators will slowly get out of sync anyway because sleeping isn't a very accurate thing AFAIK. But I can also add some randomness if you want.
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@73
PS2, Line 73: copyOfLocks = locks_.entrySet().stream().collect(
> I think sleeping for the full interval will cause drift of when the heartbe
Done
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@74
PS2, Line 74: Collectors.toMap(e -> e.getKey(), e -> e.getValue()));
> I wonder if we want to catch Throwable as well. I don't think that should g
Done
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@86
PS2, Line 86: durationOfHeartbeatingMillis + " milliseconds.");
> We should probably log this (maybe just at the info level). I think it shou
Added logging at the MetastoreShim side.
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@87
PS2, Line 87: }
> This is actually a little weird because we're removing the list that we're
I create a copy of the outer object's map in the synchronized block then iterate over the copy outside of the critical section. See L59 and L65.
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@116
PS2, Line 116: if (oldEnough(entry)) {
> Should we log if the transaction was not present? My understanding is that
Added log statement.
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Aug 2019 17:59:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Yongzhi Chen (Code Review)" <ge...@cloudera.org>.
Yongzhi Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13968/2/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/13968/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1683
PS2, Line 1683:
Do we have commitTransaction calls somewhere that actively call the transactionKeepalive_.deleteTransaction(txnId); method(instead of using keepalive to detect and remove)?
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1726
PS2, Line 1726: lockComponent.setType(LockType.EXCLUSIVE);
Does hive have code to handle deadlock prevention?
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Aug 2019 22:42:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
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/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
IMPALA-8637: Implement transaction handling and locking for ACID queries
Adds background thread to the Frontend that periodically heartbeats
the opened transactions and locks. Most of the logic is implemented
in the newly added TransactionKeepalive class.
TransactionKeepalive keeps track of the creation time of the
transactions and locks, and only heartbeat them if they are older
then the sleep interval. This way we don't heartbeat short-running
queries unnecessarily.
Testing:
Added an exhaustive test that checks heartbeating. It creates a long
running transaction with sleep(). While the transaction is executing
the test periodically checks whether there is an OPEN transaction that
has sent a heartbeat to HMS. If it found one then the test succeeds.
TODOs:
* add metrics
Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Reviewed-on: http://gerrit.cloudera.org:8080/13968
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
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/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
M tests/query_test/test_acid.py
9 files changed, 537 insertions(+), 19 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 13
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 12: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 12
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Aug 2019 21:48:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 10: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13968/3/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/13968/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@522
PS3, Line 522: client.heartbeat(txnId, lockId);
> Currently not. Do you know how we do that in the Frontend, e.g.? I'm not su
Ah yeah, you're totally right. Thanks for fixing.
http://gerrit.cloudera.org:8080/#/c/13968/10/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/13968/10/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@567
PS10, Line 567: } catch (InterruptedException e) {
Leave a comment here to explain why it isn't a bug (empty catch blocks automatically look like a bug to me)
http://gerrit.cloudera.org:8080/#/c/13968/10/tests/query_test/test_acid.py
File tests/query_test/test_acid.py:
http://gerrit.cloudera.org:8080/#/c/13968/10/tests/query_test/test_acid.py@116
PS10, Line 116: if self._any_open_heartbeated_transaction_since(latest_open_txn):
Nice!
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Aug 2019 22:42:13 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 2:
(11 comments)
I did a pass over this, tried to mainly focus on correctness and observability. I have some suggestions, you may have already had some of the same ideas and be planning to implement but I thought I'd mention them now to reduce the number of round-trips.
http://gerrit.cloudera.org:8080/#/c/13968/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13968/2//COMMIT_MSG@19
PS2, Line 19: * add some tests
Metrics for the following would be good (just some ideas)
* number of open transactions
* number of open locks
* statistics about heartbeat RPC time
* transactions added over lifetime of process
* transactions timed out over lifetime of process (would help us to see if this was an issue in aggregated)
I think the metrics for frontend code are trickier than in the backend. One idea is to do a JNI call to the backend to update the metrics after each round of heartbeats.
http://gerrit.cloudera.org:8080/#/c/13968/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/13968/2/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@516
PS2, Line 516: } catch (NoSuchLockException e) {
Should we log these? I also asked this in the caller, maybe this is a better place to log since we can record the reason it failed.
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
File fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java:
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@44
PS1, Line 44: private boolean stopped_ = false;
Observability to see # of transactions, heartbeat time, etc?
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@65
PS1, Line 65: for (Map.Entry<Long, List<Long>> entry : copyOfTxnsAndLocks.entrySet()) {
I think we could drift behind if the heartbeats take a long time.
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@66
PS1, Line 66: Long transactionId = entry.getKey();
> Do you plan to be more specific about the exceptions caught?
Should we catch all throwables?
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
File fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java:
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@54
PS2, Line 54: try {
I think it would be worth logging some information each time this thread wakes up, e.g. how how many locks are active and how long sending out the heartbeats took. If the frequency is at least a minute, this might not be too bad in terms of verbosity. We could also only log occasionally.
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@68
PS2, Line 68: sendHeartbeat(hmsClient, transactionId, lockId);
I'm curious if the HMS supports or plans to support a bulk heartbeat API where we could just provide the full list of locks to heartbeat.
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@73
PS2, Line 73: Thread.sleep(SLEEP_INTERVAL_SECONDS * 1000);
I think sleeping for the full interval will cause drift of when the heartbeats happen, if the actual heartbeats take a significant amount of time, which seems possible.
I'd suggest getting the time from the monotonic clock at the top of the loop and calculating the sleep duration based on that, e.g.something like below (with the correct unit conversions)
sleep(heartbeatStartTime + SLEEP_INTERVAL_SECONDS - now())
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@74
PS2, Line 74: } catch (Exception e) {
I wonder if we want to catch Throwable as well. I don't think that should generally happen but it would result in this thread failing and the process being in a broken state.
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@86
PS2, Line 86: // Transaction or lock doesn't exist anymore, stop heartbeating it.
We should probably log this (maybe just at the info level). I think it should be fairly rare in the normal course of things but would be very useful to debug aborted transactions.
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@116
PS2, Line 116: txnsAndLocks_.remove(transactionId);
Should we log if the transaction was not present? My understanding is that this *can* occur if the transaction was timed out.
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Aug 2019 20:43:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13968/8/tests/query_test/test_acid.py
File tests/query_test/test_acid.py:
http://gerrit.cloudera.org:8080/#/c/13968/8/tests/query_test/test_acid.py@104
PS8, Line 104: #
flake8: E265 block comment should start with '# '
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 8
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Aug 2019 15:35:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 5:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/4155/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Aug 2019 15:50:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 9:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4174/ : 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/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 9
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Aug 2019 16:47:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13968/10/tests/query_test/test_acid.py
File tests/query_test/test_acid.py:
http://gerrit.cloudera.org:8080/#/c/13968/10/tests/query_test/test_acid.py@105
PS10, Line 105: latest_open_txn
> nit: I don't like the name too much, it made me assume that it is a txn id
Done
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Aug 2019 14:45:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 11: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13968/10/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/13968/10/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@567
PS10, Line 567: } catch (InterruptedException e) {
> Added a comment. I was wondering would it make sense to make acquireLock()
I agree with the reasoning
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 11
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Aug 2019 14:47:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13968/3/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/13968/3/be/src/service/client-request-state.cc@1129
PS3, Line 1129: int64_t txn_id = GetTransactionId();
> Maybe log the error? Should be pretty rare and might be useful on the off c
Done
http://gerrit.cloudera.org:8080/#/c/13968/3/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/13968/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@522
PS3, Line 522: client.heartbeat(txnId, lockId);
> We get the query id as part of the log message, right?
Currently not. Do you know how we do that in the Frontend, e.g.? I'm not sure if we could do the same trick since we are on a different thread here that heartbeats multiple in behalf of multiple queries.
I added a HeartbeatContext object for each transaction and lock in the map, and I explicitly added the query_id to the log messages in TransactionKeepalive.
http://gerrit.cloudera.org:8080/#/c/13968/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@553
PS3, Line 553: lockRequestBuilder.setTransactionId(txnId);
: for (LockComponent lockComponent :
> lock() may also return state WAITING according to https://github.com/apache
Thanks for catching this. Added some retrying and waiting logic to this function.
http://gerrit.cloudera.org:8080/#/c/13968/3/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/13968/3/fe/src/main/java/org/apache/impala/service/Frontend.java@265
PS3, Line 265:
> Is there a reason this liven in Frontend class? The reason I ask is that I
We talked about this offline. So the original concept was that the Coordinator is responsible for managing transactions, locks, and heartbeats.
But maybe for some statements it'd make sense to execute them on the Catalog side, e.g. DROP and TRUNCATE. Basically we could just put the TransactionKeepalive class to common and have both the Frontend and CatalogOpExecutor could have an instance of it.
http://gerrit.cloudera.org:8080/#/c/13968/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1743
PS3, Line 1743: ().getName(
> Shouldn't this be SHARED_WRITE?
No, SHARED_WRITE is for UPDATE and DELETE.
https://github.com/apache/hive/blob/959ebeb680b07d59f4f55939862ebbc2d7f16a92/ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java#L2873
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
File fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java:
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@73
PS2, Line 73: private class DaemonThread implements Runnable {
> Yeah I think adding some jitter would be useful, e.g. if you sleep for a ra
Added some jitter as suggested.
Yeah actually I'm familiar with the session expiry logic and initially I was thinking about implementing stg similar. But I wanted to start with a simple solution. Also, currently we only open transactions for INSERTs.
Added a TODO to implement a smarter solution when we start putting queries into transactions.
Btw, even when we put every query into a transaction it doesn't necessarily mean that we open a transaction for each one of them, i.e. a bunch of queries could just re-use the same transaction id. That'd complicate locking a bit though.
http://gerrit.cloudera.org:8080/#/c/13968/3/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
File fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java:
http://gerrit.cloudera.org:8080/#/c/13968/3/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@93
PS3, Line 93: pyOf
> I think that this should be error as the program really shouldn't get here.
Done
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Aug 2019 15:00:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Yongzhi Chen, Gabor Kaszab, Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13968
to look at the new patch set (#4).
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
IMPALA-8637: Implement transaction handling and locking for ACID queries
Adds background thread to the Frontend that periodically heartbeats
the opened transactions and locks. Most of the logic is implemented
in the newly added TransactionKeepalive class.
TransactionKeepalive keeps track of the creation time of the
transactions and locks, and only heartbeat them if they are older
then the sleep interval. This way we don't heartbeat short-running
queries unnecessarily.
TODOs:
* add tests
* add metrics
Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
---
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
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/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
8 files changed, 464 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/13968/4
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 12:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4761/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 12
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Aug 2019 17:40:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13968/7/tests/query_test/test_acid.py
File tests/query_test/test_acid.py:
http://gerrit.cloudera.org:8080/#/c/13968/7/tests/query_test/test_acid.py@124
PS7, Line 124: def _any_open_heartbeated_transaction(self):
: hive_transactions = self.run_stmt_in_hive("SHOW TRANSACTIONS")
: for transaction_line in hive_transactions.split('\n'):
: transaction_columns = transaction_line.split(',')
: if len(transaction_columns) < 4:
: continue
: transaction_state = transaction_columns[1]
: transaction_start_time = transaction_columns[2]
: transaction_last_heartbeat = transaction_columns[3]
: if transaction_state == 'OPEN':
: if transaction_start_time != transaction_last_heartbeat:
: return True
: return False
My concern here is that as our experience is that HMS never aborts transactions due to missing heartbeats, so there can be quite old open transactions, and if there was ever any heartbeat sent for them, they will satisfy this condition. This seems possible to me for long running Hive inserts/compactions, so this function can return true even if Impala does not send heartbeats at all.
I would do the following changes to make the test stronger:
- get the count or set of open_heartbeated transacations instead of just checking their existence
- make the test sequential
- check the count/set before starting the query, and then expect it to be increased/have a new transaction at one point
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Aug 2019 14:03:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13968/3/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/13968/3/fe/src/main/java/org/apache/impala/service/Frontend.java@265
PS3, Line 265: private final TransactionKeepalive transactionKeepalive_;
Is there a reason this liven in Frontend class? The reason I ask is that I started working on DROP ACID tables and the most logical location to do the locking is in CatalogOpExecutor. As I see from there I could create the lock, but won't be able to start hearbeating it since TransactionKeepalive lives in Frontend and that is not available in that level.
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Aug 2019 14:11:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4108/ : 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/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 Jul 2019 19:24:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Yongzhi Chen, Gabor Kaszab, Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13968
to look at the new patch set (#9).
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
IMPALA-8637: Implement transaction handling and locking for ACID queries
Adds background thread to the Frontend that periodically heartbeats
the opened transactions and locks. Most of the logic is implemented
in the newly added TransactionKeepalive class.
TransactionKeepalive keeps track of the creation time of the
transactions and locks, and only heartbeat them if they are older
then the sleep interval. This way we don't heartbeat short-running
queries unnecessarily.
Testing:
Added an exhaustive test that checks heartbeating. It creates a long
running transaction with sleep(). While the transaction is executing
the test periodically checks whether there is an OPEN transaction that
has sent a heartbeat to HMS. If it found one then the test succeeds.
TODOs:
* add metrics
Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
---
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
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/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
M tests/query_test/test_acid.py
9 files changed, 530 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/13968/9
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 9
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4175/ : 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/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Aug 2019 16:47:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 12: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 12
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Aug 2019 17:40:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 7:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/4171/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Aug 2019 14:05:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 6: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Aug 2019 11:30:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 1:
(16 comments)
Hey Zoltan,
I ran through the code, left some comments but mostly question for my education.
http://gerrit.cloudera.org:8080/#/c/13968/1/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/13968/1/fe/src/main/java/org/apache/impala/service/Frontend.java@267
PS1, Line 267: TransactionKeepalive
Don't you have to import this class?
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/Frontend.java@302
PS1, Line 302: if (MetastoreShim.getMajorVersion() > 2) {
: transactionKeepalive_ = new TransactionKeepalive(metaStoreClientPool_);
: } else {
: transactionKeepalive_ = null;
: }
Do you plan to move this to MetastoreShim:
transactionKeepalive_ = MetastoreShim.getStransactionKeepalive(metaStoreClientPool_);
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1308
PS1, Line 1308: // TODO (IMPALA-8788): should load table write ids in transaction context.
No need for this TODO anymore, right?
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1314
PS1, Line 1314: createLock(txnId, targetTable);
I'm not that familiar with HMS locks: Don't you have to release this lock somewhere?
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1688
PS1, Line 1688: Long txnId = transactionId;
This variable only needed for logging. Could you do this conversion inline in L1689?
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1717
PS1, Line 1717: IMetaStoreClient hmsClient = this.metaStoreClientPool_.getClient().getHiveClient();
No need to create a variable here. Actually you can inline this into L1729.
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/JniFrontend.java@652
PS1, Line 652: * @throws TransactionException
It would be nice to comment in what cases this throws.
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
File fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java:
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@24
PS1, Line 24: import java.util.Map;
Does HashMap pull in Map as well?
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@37
PS1, Line 37: private final long SLEEP_INTERVAL_SECONDS = 60;
Should this be configurable?
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@52
PS1, Line 52: Map<Long, List<Long>> copyOfTxnsAndLocks;
Do you know how many of these can we expect in a regular scenario and maximum?
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@55
PS1, Line 55: txnsAndLocks_
if txnsAndLocks_.size() is zero then you can skip running the rest below, right? (except calling sleep())
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@66
PS1, Line 66: } catch (Exception e) {
Do you plan to be more specific about the exceptions caught?
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@76
PS1, Line 76: long lockIdVal = lockId != null ? lockId : 0;
Does zero mean undefined/invalid for lock IDs? (I recall that for writeIds -1 is the invalid value)
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@77
PS1, Line 77: hmsClient.heartbeat(transactionId, lockIdVal);
Is that possible that heartbeat fails as the transaction or the lock no longer exists in HMS (e.g. because it finished meanwhile.)? Would it throw an exception that we log as warning? Is there a specific type of exception for this that we could omit?
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@86
PS1, Line 86: metaStoreClientPool_ = metaStoreClientPool;
Precondition somewhere that this is not null.
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@104
PS1, Line 104: txnsAndLocks_.remove(transactionId);
Shouldn't the transaction be removed from HMS as well at some point?
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Aug 2019 09:50:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Gabor Kaszab, Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13968
to look at the new patch set (#2).
Change subject: WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
Initial prototype for locking tables and heartbeating the transaction
and the lock.
Currently only works for INSERT statement, and only locks the target
table.
New TransactionKeepalive class starts a daemon thread that
periodically heartbeats the registered transactions and locks.
TODO in following PSs:
* add some tests
Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
---
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
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/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
8 files changed, 281 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/13968/2
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13968/3/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/13968/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@553
PS3, Line 553: LockResponse lockResponse = client.lock(lockRequest);
: return lockResponse.getLockid();
lock() may also return state WAITING according to https://github.com/apache/hive/blob/212b4281ee6f138fe0835929531fbf3c36505bff/standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift#L169
If I understand correctly, the idea is that the lock request (potentially for multiple tables) is put to a queue if there is a conflicting lock, and the client should poll HMS with check_lock calls. So we already have the lock_id (so it can be aborted), but the lock is not "ours" yet, so starting using it is not valid.
I would vote for a simple solution for now, for example aborting the query in this case, or doing 1-2 check_lock and aborting it if we still couldn't get the lock.
http://gerrit.cloudera.org:8080/#/c/13968/3/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/13968/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1743
PS3, Line 1743: SHARED_READ
Shouldn't this be SHARED_WRITE?
http://gerrit.cloudera.org:8080/#/c/13968/3/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
File fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java:
http://gerrit.cloudera.org:8080/#/c/13968/3/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@93
PS3, Line 93: warn
I think that this should be error as the program really shouldn't get here.
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Aug 2019 17:24:36 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13968/7/tests/query_test/test_acid.py
File tests/query_test/test_acid.py:
http://gerrit.cloudera.org:8080/#/c/13968/7/tests/query_test/test_acid.py@124
PS7, Line 124: def _latest_open_transaction(self):
: max_start = 0
: for txn in self._get_transactions():
: start = txn['start_time']
: if start > max_start:
: max_start = start
: return max_start
:
: def _any_open_heartbeated_transaction_since(self, since_start_time):
: for txn in self._get_transactions():
: if txn['state'] == 'OPEN':
: start = txn['start_time']
: if start
> My concern here is that as our experience is that HMS never aborts transact
In the new PS I only care about transactions that were opened by Impala.
Also, I search for the latest open transaction in the beginning and wait for the newly created transaction to send a heartbeat.
http://gerrit.cloudera.org:8080/#/c/13968/9/tests/query_test/test_acid.py
File tests/query_test/test_acid.py:
http://gerrit.cloudera.org:8080/#/c/13968/9/tests/query_test/test_acid.py@104
PS9, Line 104: i
> flake8: E265 block comment should start with '# '
Done
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Aug 2019 16:10:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
File fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java:
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@48
PS2, Line 48: private Map<Long, List<Long>> txnsAndLocks_ = new HashMap<>();
maybe for a later commit:
I think it would be beneficial to avoid sending heartbeats for transactions that do not need it (because they started/hearbeated not too long ago). This could be done by saving the transaction creation time / last heartbeat time, and skip the heartbeat if now() - heartbeat_time < some_constant.
If SELECTs will also open transactions, then there will be probably a large number of short lived transaction that shouldn't send any heartbeats. As far as I know every heartbeat leads to a DB update in HMS, so these are not very cheap RPCs.
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@67
PS2, Line 67: for (Long lockId : entry.getValue()) {
Are you sure that we have to heartbeat every lock for a given transaction? This seems counter intuitive to me, so I think a comment about it would be useful.
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@73
PS2, Line 73: Thread.sleep(SLEEP_INTERVAL_SECONDS * 1000);
> I think sleeping for the full interval will cause drift of when the heartbe
Doing this way makes me a bit worried about multiple coordinators starting at the same time and then bombing HMS with heartbeats at the same time. Waiting a random time at the beginning could help with this. Probably this is not critical now, but may worth a TODO.
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Aug 2019 15:24:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 2:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/4118/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Aug 2019 18:08:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4748/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Aug 2019 07:25:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13968/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13968/2//COMMIT_MSG@19
PS2, Line 19: * add tests
> Added logs for observability but I agree that such metrics would be very us
Yep that seems fine, best to avoid mega-CRs if possible.
http://gerrit.cloudera.org:8080/#/c/13968/3/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/13968/3/be/src/service/client-request-state.cc@1129
PS3, Line 1129: discard_result(frontend_->UnregisterTransaction(GetTransactionId()));
Maybe log the error? Should be pretty rare and might be useful on the off chance that we have symptoms like open transactions piling up.
http://gerrit.cloudera.org:8080/#/c/13968/3/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/13968/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@522
PS3, Line 522: LOG.info(errorMsg, e);
We get the query id as part of the log message, right?
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
File fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java:
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@73
PS2, Line 73: copyOfLocks = locks_.entrySet().stream().collect(
> I added the logic requested from Tim. I think the coordinators will slowly
Yeah I think adding some jitter would be useful, e.g. if you sleep for a random amount between [0, SLEEP_INTERVAL_SECONDS] on startup and then add some randomness to the time you sleep each iteration.
You could also make things more complicated by having a separate "next heartbeat time" for each open transaction to spread out the RPCs within each interval. That does complicate the data structures since you'd want something that supports both efficient removal and efficiently finding the next transaction to heartbeat, but I think it is a better solution than doing them in one big batch.
We did something like this for session expiry in ImpalaServer. You'd probably want a priority queue to track the next transaction, then a way to mark a transaction as closed so that it can be removed from the priority queue on the next heartbeat.
Anyway, I think what we have now is OK and will work in practice unless there's some really extreme load on the HMS, since at least we're only doing one RPC per coordinator at a time, so I'll leave it up to you about whether you want to try and improve this.
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Aug 2019 23:07:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13968/9/tests/query_test/test_acid.py
File tests/query_test/test_acid.py:
http://gerrit.cloudera.org:8080/#/c/13968/9/tests/query_test/test_acid.py@104
PS9, Line 104: #
flake8: E265 block comment should start with '# '
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 9
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Aug 2019 16:06:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13968/5/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/13968/5/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@566
PS5, Line 566: Thread.sleep(retryWaitSeconds * 1000);
TODO in next PS: put this Thread.sleep() before checkLock()
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Aug 2019 16:09:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
File fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java:
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@44
PS2, Line 44: private boolean stopped_ = false;
Never used?
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@87
PS2, Line 87: TransactionKeepalive.this.deleteTransaction(transactionId);
This is actually a little weird because we're removing the list that we're iterating over from the map - might be better to bubble the failure up to the above function and have it early-exit out of the loop over transactions.
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Aug 2019 20:50:43 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 11
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Aug 2019 17:20:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 3:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/4125/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Aug 2019 18:25:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Yongzhi Chen, Gabor Kaszab, Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13968
to look at the new patch set (#3).
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
IMPALA-8637: Implement transaction handling and locking for ACID queries
Adds background thread to the Frontend that periodically heartbeats
the opened transactions and locks. Most of the logic is implemented
in the newly added TransactionKeepalive class.
TransactionKeepalive keeps track of the creation time of the
transactions and locks, and only heartbeat them if they are older
then the sleep interval. This way we don't heartbeat short-running
queries unnecessarily.
TODOs:
* add tests
* add metrics
Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
---
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
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/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
8 files changed, 397 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/13968/3
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Yongzhi Chen, Gabor Kaszab, Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13968
to look at the new patch set (#8).
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
IMPALA-8637: Implement transaction handling and locking for ACID queries
Adds background thread to the Frontend that periodically heartbeats
the opened transactions and locks. Most of the logic is implemented
in the newly added TransactionKeepalive class.
TransactionKeepalive keeps track of the creation time of the
transactions and locks, and only heartbeat them if they are older
then the sleep interval. This way we don't heartbeat short-running
queries unnecessarily.
Testing:
Added an exhaustive test that checks heartbeating. It creates a long
running transaction via a debug action injects sleeps into the
HdfsTableSink. While the transaction is executing the test periodically
checks whether there is an OPEN transaction that has sent a heartbeat
to HMS. If it found one then the test succeeds.
TODOs:
* add metrics
Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
---
M be/src/exec/hdfs-table-sink.cc
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
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/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
M tests/query_test/test_acid.py
10 files changed, 533 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/13968/8
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 8
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 10: Code-Review+1
(2 comments)
I have some minor nits. Up to you if you take care of them.
http://gerrit.cloudera.org:8080/#/c/13968/10/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
File fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java:
http://gerrit.cloudera.org:8080/#/c/13968/10/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java@343
PS10, Line 343: createLock
nit: "acquireLock is not supported" instead, right?
http://gerrit.cloudera.org:8080/#/c/13968/10/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
File fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java:
http://gerrit.cloudera.org:8080/#/c/13968/10/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@81
PS10, Line 81: Thread.sleep
nit: a comment would be nice to explain why we sleep for a random time when the thread starts.
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Aug 2019 08:14:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 11:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13968/10/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
File fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java:
http://gerrit.cloudera.org:8080/#/c/13968/10/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java@343
PS10, Line 343: acquireLoc
> nit: "acquireLock is not supported" instead, right?
Done
http://gerrit.cloudera.org:8080/#/c/13968/10/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/13968/10/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@567
PS10, Line 567: } catch (InterruptedException e) {
> Leave a comment here to explain why it isn't a bug (empty catch blocks auto
Added a comment. I was wondering would it make sense to make acquireLock() interruptible, but then I convinced myself that the user of this method should just set the parameters to control wait duration.
http://gerrit.cloudera.org:8080/#/c/13968/10/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
File fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java:
http://gerrit.cloudera.org:8080/#/c/13968/10/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@81
PS10, Line 81: // Let's sle
> nit: a comment would be nice to explain why we sleep for a random time when
Added comment.
http://gerrit.cloudera.org:8080/#/c/13968/10/tests/query_test/test_acid.py
File tests/query_test/test_acid.py:
http://gerrit.cloudera.org:8080/#/c/13968/10/tests/query_test/test_acid.py@111
PS10, Line 111: handle = self.execute_query_async(
> nit: indent 4
Done
http://gerrit.cloudera.org:8080/#/c/13968/10/tests/query_test/test_acid.py@116
PS10, Line 116: while attempt < MAX_ATTEMPTS:
> Nice!
Thanks!
http://gerrit.cloudera.org:8080/#/c/13968/10/tests/query_test/test_acid.py@121
PS10, Line 121: time.sleep(20)
> Can we do it in a try: finally: block? If there is an exception for some re
Done
http://gerrit.cloudera.org:8080/#/c/13968/10/tests/query_test/test_acid.py@140
PS10, Line 140: turn False
> nit: _get_impala_transactions would be more precise
Done
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 11
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Aug 2019 14:42:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 2:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/13968/1/be/src/service/frontend.h
File be/src/service/frontend.h:
http://gerrit.cloudera.org:8080/#/c/13968/1/be/src/service/frontend.h@172
PS1, Line 172:
> line has trailing whitespace
Done
http://gerrit.cloudera.org:8080/#/c/13968/1/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/13968/1/fe/src/main/java/org/apache/impala/service/Frontend.java@267
PS1, Line 267: d(AuthorizationFacto
> Don't you have to import this class?
It's in the same package so you don't need to import.
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/Frontend.java@302
PS1, Line 302: } else {
: transactionKeepalive_ = null;
: }
: }
:
> Do you plan to move this to MetastoreShim:
I'd rather keep it so we know that it can be null.
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1308
PS1, Line 1308: queryCtx.setTransaction_id(txnId);
> No need for this TODO anymore, right?
It's still a TODO because we fetch the valid write ids outside of transaction context in L1278:
StmtTableCache stmtTableCache = metadataLoader.loadTables(stmt);
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1314
PS1, Line 1314: insertStmt.setWriteId(writeId);
> I'm not that familiar with HMS locks: Don't you have to release this lock s
Locks are automatically released by commit/abort if they are associated with the transaction.
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1688
PS1, Line 1688: public void unregisterTransaction(long txnId) {
> This variable only needed for logging. Could you do this conversion inline
Changed parameter name to txnId and use it everywhere.
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1717
PS1, Line 1717: FeTable targetTable, boolean isOverwrite) throws TransactionException {
> No need to create a variable here. Actually you can inline this into L1729.
This code changed significantly.
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/JniFrontend.java@652
PS1, Line 652: */
> It would be nice to comment in what cases this throws.
Thanks for catching it, actually it cannot throw.
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
File fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java:
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@24
PS1, Line 24: import java.util.Map;
> Does HashMap pull in Map as well?
No, in Java you need to import all your dependencies explicitly (unless they are in the same package).
Or I could use import java.util.*; but it's considered bad practice AFAIK.
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@37
PS1, Line 37: public class TransactionKeepalive {
> Should this be configurable?
Yeah, actually it should be calculated from metastore.txn.timeout. Added TODO about it.
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@52
PS1, Line 52: public void run() {
> Do you know how many of these can we expect in a regular scenario and maxim
Since we don't recommend Impala as an OLTP database and there are multiple coordinators, I think a couple dozen at most. And most of the time 1 or 2.
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@55
PS1, Line 55: ransactions a
> if txnsAndLocks_.size() is zero then you can skip running the rest below, r
Yeah, I've put them into an if.
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@66
PS1, Line 66: Long transactionId = entry.getKey();
> Do you plan to be more specific about the exceptions caught?
I don't want to terminate this daemon thread so I just catch everything here. The code got more specific in the MetastoreShim and sendHeartbeat. We swallow the exceptions related to non-existent locks and transactions and remove them from the map of this class.
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@76
PS1, Line 76: }
> Does zero mean undefined/invalid for lock IDs? (I recall that for writeIds
0 means no locks have been acquired.
https://github.com/apache/hive/blob/master/standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java#L3143-L3146
In Hive ACID the value 0 is also invalid for writeIds. We just chose the value -1 in Impala because we use at other places as well.
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@77
PS1, Line 77: }
> Is that possible that heartbeat fails as the transaction or the lock no lon
Yeah we logged them as warning. I changed the behavior in the new PS.
HMSClient.heartbeat() can throw NoSuchLockException, NoSuchTxnException, TxnAbortedException when something doesn't exist anymore. MetastoreShim.heartbeat() returns false if any of those is being thrown. When this happens we remove the transaction from our map.
When HMSClient.heartbeat() throws the general TException MetastoreShim.heartbeat() converts it into a TransactionException and we log.warn() it.
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@86
PS1, Line 86: // Transaction or lock doesn't exist anymore, stop heartbeating it.
> Precondition somewhere that this is not null.
Done
http://gerrit.cloudera.org:8080/#/c/13968/1/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@104
PS1, Line 104:
> Shouldn't the transaction be removed from HMS as well at some point?
At this point the transaction should have been already committed or aborted.
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Aug 2019 17:39:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13968/1/be/src/service/frontend.h
File be/src/service/frontend.h:
http://gerrit.cloudera.org:8080/#/c/13968/1/be/src/service/frontend.h@172
PS1, Line 172:
line has trailing whitespace
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 Jul 2019 18:43:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 3:
I think this is getting close. Would really prefer some kind of basic test, but as far as the code goes I think starting to look good -
I agree with Csaba's last round of comments.
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Aug 2019 23:08:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4154/ : 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/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Aug 2019 15:40:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4180/ : 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/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 11
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Aug 2019 15:23:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 2:
(2 comments)
Did a quick run through again. I most probably will switch to spectator mode and follow the review for my education.
http://gerrit.cloudera.org:8080/#/c/13968/2/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/13968/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1718
PS2, Line 1718: List<LockComponent> lockComponents = new ArrayList<>();
Can you give tables.size() to the arraylist constructor?
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
File fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java:
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@58
PS2, Line 58: stopped_
as Tim noticed, this is never true, right?
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Aug 2019 14:18:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Yongzhi Chen, Gabor Kaszab, Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13968
to look at the new patch set (#5).
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
IMPALA-8637: Implement transaction handling and locking for ACID queries
Adds background thread to the Frontend that periodically heartbeats
the opened transactions and locks. Most of the logic is implemented
in the newly added TransactionKeepalive class.
TransactionKeepalive keeps track of the creation time of the
transactions and locks, and only heartbeat them if they are older
then the sleep interval. This way we don't heartbeat short-running
queries unnecessarily.
TODOs:
* add tests
* add metrics
Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
---
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
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/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
8 files changed, 465 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/13968/5
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 8:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/4173/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 8
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Aug 2019 16:16:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
File fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java:
http://gerrit.cloudera.org:8080/#/c/13968/2/fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java@67
PS2, Line 67: for (Long lockId : entry.getValue()) {
> Are you sure that we have to heartbeat every lock for a given transaction?
I checked what Hive does, and it only sends heartbeats for the transaction (with dummy lock id 0) if there is a transaction. This is the expected way to do it, the metastore will do extra work if the lock id is not 0.
client side:
https://github.com/apache/hive/blob/d7475aa98f6a2fc813e2e1c0ad99f902cb28cc00/ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java#L566
metastore side:
This is where the heartbeat begins:
https://github.com/apache/hive/blob/d7475aa98f6a2fc813e2e1c0ad99f902cb28cc00/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java#L2823
updating a lock's timestamp:
https://github.com/apache/hive/blob/d7475aa98f6a2fc813e2e1c0ad99f902cb28cc00/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java#L4542
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Aug 2019 16:07:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Yongzhi Chen, Gabor Kaszab, Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13968
to look at the new patch set (#11).
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
IMPALA-8637: Implement transaction handling and locking for ACID queries
Adds background thread to the Frontend that periodically heartbeats
the opened transactions and locks. Most of the logic is implemented
in the newly added TransactionKeepalive class.
TransactionKeepalive keeps track of the creation time of the
transactions and locks, and only heartbeat them if they are older
then the sleep interval. This way we don't heartbeat short-running
queries unnecessarily.
Testing:
Added an exhaustive test that checks heartbeating. It creates a long
running transaction with sleep(). While the transaction is executing
the test periodically checks whether there is an OPEN transaction that
has sent a heartbeat to HMS. If it found one then the test succeeds.
TODOs:
* add metrics
Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
---
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
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/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
M tests/query_test/test_acid.py
9 files changed, 537 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/13968/11
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 11
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Yongzhi Chen, Gabor Kaszab, Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13968
to look at the new patch set (#7).
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
IMPALA-8637: Implement transaction handling and locking for ACID queries
Adds background thread to the Frontend that periodically heartbeats
the opened transactions and locks. Most of the logic is implemented
in the newly added TransactionKeepalive class.
TransactionKeepalive keeps track of the creation time of the
transactions and locks, and only heartbeat them if they are older
then the sleep interval. This way we don't heartbeat short-running
queries unnecessarily.
Testing:
Added an exhaustive test that checks heartbeating. It creates a long
running transaction via a debug action injects sleeps into the
HdfsTableSink. While the transaction is executing the test periodically
checks whether there is an OPEN transaction that has sent a heartbeat
to HMS. If it found one then the test succeeds.
TODOs:
* add metrics
Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
---
M be/src/exec/hdfs-table-sink.cc
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
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/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
M tests/query_test/test_acid.py
10 files changed, 516 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/13968/7
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Yongzhi Chen, Gabor Kaszab, Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13968
to look at the new patch set (#10).
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
IMPALA-8637: Implement transaction handling and locking for ACID queries
Adds background thread to the Frontend that periodically heartbeats
the opened transactions and locks. Most of the logic is implemented
in the newly added TransactionKeepalive class.
TransactionKeepalive keeps track of the creation time of the
transactions and locks, and only heartbeat them if they are older
then the sleep interval. This way we don't heartbeat short-running
queries unnecessarily.
Testing:
Added an exhaustive test that checks heartbeating. It creates a long
running transaction with sleep(). While the transaction is executing
the test periodically checks whether there is an OPEN transaction that
has sent a heartbeat to HMS. If it found one then the test succeeds.
TODOs:
* add metrics
Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
---
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
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/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/service/TransactionKeepalive.java
M tests/query_test/test_acid.py
9 files changed, 530 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/13968/10
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-8637: Implement transaction handling and locking for ACID queries
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/13968 )
Change subject: IMPALA-8637: Implement transaction handling and locking for ACID queries
......................................................................
Patch Set 10: Code-Review+2
(4 comments)
Thanks for upgrading the tests!
I have some nits, otherwise it is ok for me.
http://gerrit.cloudera.org:8080/#/c/13968/10/tests/query_test/test_acid.py
File tests/query_test/test_acid.py:
http://gerrit.cloudera.org:8080/#/c/13968/10/tests/query_test/test_acid.py@105
PS10, Line 105: latest_open_txn
nit: I don't like the name too much, it made me assume that it is a txn id instead of a timestamp. E.g. last_open_txn_start_time would be more descriptive.
http://gerrit.cloudera.org:8080/#/c/13968/10/tests/query_test/test_acid.py@111
PS10, Line 111: "insert into {} values (sleep(200000))".format(dummy_tbl))
nit: indent 4
http://gerrit.cloudera.org:8080/#/c/13968/10/tests/query_test/test_acid.py@121
PS10, Line 121: self.client.cancel(handle)
Can we do it in a try: finally: block? If there is an exception for some reason this long running query could affect some other tests that use metrics.
http://gerrit.cloudera.org:8080/#/c/13968/10/tests/query_test/test_acid.py@140
PS10, Line 140: _get_transactions
nit: _get_impala_transactions would be more precise
--
To view, visit http://gerrit.cloudera.org:8080/13968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa37899b24aa114be642bf8772b4e0f882865cfa
Gerrit-Change-Number: 13968
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy <bo...@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: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Aug 2019 17:09:05 +0000
Gerrit-HasComments: Yes