You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Gabor Kaszab (Code Review)" <ge...@cloudera.org> on 2019/08/01 09:50:32 UTC
[Impala-ASF-CR] WIP IMPALA-8637: Implement transaction handling and locking for ACID queries
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