Bikramjeet Vig 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: (9 comments) Looks good, just adding a few more nits http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/exec/kudu-table-sink.h File be/src/exec/kudu-table-sink.h: http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/exec/kudu-table-sink.h@107 PS5, Line 107: /// Returns TRUE if it's in Kudu transaction. nit: add: valid only after Open() succeeds http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/exec/kudu-table-sink.h@123 PS5, Line 123: std::string kudu_txn_token_ > nit: is it possible to add 'const' specifier and initialize the token in th Deserialize can return an error status which we cant propagate during initialization, therefore anything like this is usually deferred to the Open() call 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(); : } : LOG(ERROR) << "ERROR Finalizing DML: " << status.GetDetail(); : return status; : } : if (InTransaction()) { : // UpdateCatalog() succeeded and already committed the transaction for us. : int64_t txn_id = GetTransactionId(); : if (!frontend_->UnregisterTransaction(txn_id).ok()) { : LOG(ERROR) << Substitute("Failed to unregister transaction $0", txn_id); : } : ClearTransactionState(); : query_events_->MarkEvent("Transaction committed"); : } else if (InKuduTransaction()) { : CommitKuduTransaction(); : } wondering if we can consolidate the methods for Transactions and let them take care of doing the right thing for hive and kudu. will make it easier to read. eg L1331-L1341 can be taken care of by FinalizeTransaction L1323-L1327 can be taken care of by a single AbortTransaction() http://gerrit.cloudera.org:8080/#/c/17553/5/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/17553/5/common/thrift/ImpalaService.thrift@673 PS5, Line 673: KUDU_TRANSACTION nit: how about ENABLE_KUDU_TRANSACTION http://gerrit.cloudera.org:8080/#/c/17553/5/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: http://gerrit.cloudera.org:8080/#/c/17553/5/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@1124 PS5, Line 1124: setTransactionToken nit: maybe call it setKuduTransactionToken http://gerrit.cloudera.org:8080/#/c/17553/5/fe/src/main/java/org/apache/impala/planner/TableSink.java File fe/src/main/java/org/apache/impala/planner/TableSink.java: http://gerrit.cloudera.org:8080/#/c/17553/5/fe/src/main/java/org/apache/impala/planner/TableSink.java@110 PS5, Line 110: * Same as above, plus it takes an ACID write id in parameter 'writeId'. nit: update comment to mention txnToken http://gerrit.cloudera.org:8080/#/c/17553/5/fe/src/main/java/org/apache/impala/planner/TableSink.java@116 PS5, Line 116: txnToken nit: maybe name it kuduTxnToken to explicitly specify that this is kudu specific http://gerrit.cloudera.org:8080/#/c/17553/5/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/17553/5/fe/src/main/java/org/apache/impala/service/Frontend.java@1732 PS5, Line 1732: TransactionException update comment for TransactionException to include kudu cases too. http://gerrit.cloudera.org:8080/#/c/17553/5/fe/src/main/java/org/apache/impala/service/Frontend.java@2242 PS5, Line 2242: txn.close(); nit: is close() something that can be put in a finally block. Like should it always be called even it rollback or commit fails? I understand that it is Auto-closeable but if we are calling it explicitly here and not using the try-with-resource code construct then might be good to just put it in the finally block. -- 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 <wz...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Wed, 09 Jun 2021 22:00:03 +0000 Gerrit-HasComments: Yes
