Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17553 )
Change subject: IMPALA-10557: Support Kudu's multi-row transaction ...................................................................... Patch Set 13: Code-Review+1 (3 comments) Thank you very much for working on this! This patch looks good to me. http://gerrit.cloudera.org:8080/#/c/17553/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17553/8//COMMIT_MSG@19 PS8, Line 19: "insert" and "CTAS" > Sorry, CTAS is already supported. Will add test case for CTAS and update co Yep, I was trying to understand whether not supporting multi-row Kudu transactions was something deliberate or the presence of CTAS for Kudu was just overlooked, and I was wandering whether it makes sense to support multi-row transactions in the context of CTAS queries for Impala+Kudu. Thank you for adding support for that! I think there is value in that for Impala+Kudu users. http://gerrit.cloudera.org:8080/#/c/17553/8/tests/custom_cluster/test_kudu.py File tests/custom_cluster/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/17553/8/tests/custom_cluster/test_kudu.py@336 PS8, Line 336: > Added test case for heartbeating. It's possible to change --txn_keepalive_interval_ms via the `kudu tserver set_flag` CLI tool in addition to setting the flag at startup for kudu-tserver processes. Probably, the latter is less hustle since if using the `kudu tserver set_flag` approach, it would be necessary to call the following for every tablet server to set the interval to 1 second: `kudu tserver set_flag <tserver_rpc_address> txn_keepalive_interval_ms 1000` I see you already added a proper setup step into TestKuduTxnKeepalive::setup_class() method. Thank you! http://gerrit.cloudera.org:8080/#/c/17553/8/tests/custom_cluster/test_kudu.py@346 PS8, Line 346: _create_parquet_table_query = "create table {0} (a int, b string) stored as parquet" > Searched the document. INSERT IGNORE is not supported now. Thank you for the clarification and making it work for CTAS queries as well! I didn't realize INSERT_IGNORE isn't supported for Kudu+Impala. My point was to make sure that a multi-row Kudu transaction is opened by Impala only in case of INSERT/INSERT_IGNORE, but not for UPDATE/UPSERT/DELETE even if ENABLE_KUDU_TRANSACTION is set to true. Kudu would return an error if UPDATE/DELETE is performed within a multi-row transaction context. I see you already added a case that verifies that for the DELETE, so now the DELETE is now covered. If it makes sense, consider adding a sub-scenario for UPDATE/UPSERT. -- 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: 13 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: Mon, 14 Jun 2021 21:53:48 +0000 Gerrit-HasComments: Yes
