Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17553 )
Change subject: WIP IMPALA-10557: Support Kudu's multi-row transaction ...................................................................... Patch Set 5: (11 comments) Looks good! http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/exec/kudu-scan-node-base.h File be/src/exec/kudu-scan-node-base.h: http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/exec/kudu-scan-node-base.h@102 PS5, Line 102: kudu::client::KuduClient* kudu_client() { return client_.get(); } nit. should be const? http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/runtime/exec-env.h@267 PS5, Line 267: struct KuduClientPtr { : kudu::client::sp::shared_ptr<kudu::client::KuduClient> kudu_client; : }; Can we get rid off struct and use the member directly here? http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/service/client-request-state.h@768 PS5, Line 768: /// Aborts the Kudu transaction of this client request. nit May explain what can happen when the action is successful or not. It may be a good idea to return bool to indicate the status. http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/service/client-request-state.h@772 PS5, Line 772: void CommitKuduTransaction(); Same as above. http://gerrit.cloudera.org:8080/#/c/17553/1/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/17553/1/be/src/service/client-request-state.cc@825 PS1, Line 825: if (InTransaction()) { : AbortTransaction(); : } else if (InKuduTransaction()) { : AbortKuduTransaction(); : } > Currently Impala support transaction for insert-only ACID tables for insert Thanks for the clarification. Maybe we add a comment here to explain the rational that these two types of transaction do not overlap. http://gerrit.cloudera.org:8080/#/c/17553/1/be/src/service/client-request-state.cc@1624 PS1, Line 1624: transaction_closed_ If this data member is for both the impala and kudu transaction, how do we make sure only one type of transaction can be in progress at any point of time? http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/service/client-request-state.cc@1323 PS5, Line 1323: if (InTransaction()) { : AbortTransaction(); : } else if (InKuduTransaction()) { : AbortKuduTransaction(); : } nit. This code block is identical to the one at line 825-828 above. Maybe refactor. http://gerrit.cloudera.org:8080/#/c/17553/1/fe/src/main/java/org/apache/impala/common/KuduTransactionManager.java File fe/src/main/java/org/apache/impala/common/KuduTransactionManager.java: http://gerrit.cloudera.org:8080/#/c/17553/1/fe/src/main/java/org/apache/impala/common/KuduTransactionManager.java@54 PS1, Line 54: checkNotNull(txn); > Right, IIUC Impala doesn't have this capability right now, but we may consi Okay. Thanks for the explanation. http://gerrit.cloudera.org:8080/#/c/17553/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/17553/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1717 PS1, Line 1717: else if ((targetTable instanceof FeKuduTable) : && queryOptions.isKudu_transaction()) { : FeKuduTable kuduTable = (FeKuduTable) targetTable; : KuduClient client = KuduUtil.getKuduClient(kuduTable.getKuduMasterHosts()); : KuduTransaction txn = null; : try { : // Open Kudu transaction. : txn = client.newTransaction(); : timeline.markEvent("Kudu transaction opened with query id: " : + queryCtx.getQuery_id().toString()); : insertStmt.setTransactionToken(txn.serialize()); : kuduTxnManager_.addTransaction(queryCtx.getQuery_id(), txn); : queryCtx.setIs_kudu_transactional(true); : } catch (IOException e) { : if (txn != null) txn.close(); : throw new TransactionException(e.getMessage()); : } : } > Currently Kudu only support transaction for insert. They are working on upd Done http://gerrit.cloudera.org:8080/#/c/17553/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/17553/1/fe/src/main/java/org/apache/impala/service/JniFrontend.java@772 PS1, Line 772: byte[] thriftQuer > This function is called from C++ code so we cannot pass KuduTransaction obj Done http://gerrit.cloudera.org:8080/#/c/17553/1/fe/src/main/java/org/apache/impala/service/JniFrontend.java@782 PS1, Line 782: ery. > This function is called from C++ code so we cannot pass KuduTransaction obj Done -- To view, visit http://gerrit.cloudera.org:8080/17553 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I876ada48991afdff5d61b5d6a0417571aba7cb34 Gerrit-Change-Number: 17553 Gerrit-PatchSet: 5 Gerrit-Owner: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Tue, 08 Jun 2021 19:13:22 +0000 Gerrit-HasComments: Yes
