Alexey Serbin 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: (5 comments) 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@123 PS5, Line 123: std::string kudu_txn_token_ nit: is it possible to add 'const' specifier and initialize the token in the initializer's list during construction? http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/exec/kudu-table-sink.cc File be/src/exec/kudu-table-sink.cc: http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/exec/kudu-table-sink.cc@90 PS5, Line 90: else { : kudu_txn_token_ = ""; : } nit: I guess this part isn't strictly necessary given that std::string initializes as an empty string by its default constructor. http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/exec/kudu-table-sink.cc@380 PS5, Line 380: if (txn_.get() != nullptr) nit: this could be omitted. At least, this code resets 'session_' and 'client_' regardless of their current state. 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@1333 PS5, Line 1333: int64_t txn_id = GetTransactionId(); : if (!frontend_->UnregisterTransaction(txn_id).ok()) { : LOG(ERROR) << Substitute("Failed to unregister transaction $0", txn_id); : } : ClearTransactionState(); nit: I'm curious why this is performed only for Hive, but not for a Kudu transaction. Does it make sense to add a comment to clarify on this? 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@1125 PS5, Line 1125: if (kuduTxnToken != null) nit: what if txn token is null? Is that a programming error or that's one of the intended use cases here? If the former, maybe add a precondition check here? -- 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: Tue, 08 Jun 2021 23:59:38 +0000 Gerrit-HasComments: Yes
