Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17326 )
Change subject: [tests] enable using txns in TestWorkload ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/17326/1/src/kudu/integration-tests/test_workload.h File src/kudu/integration-tests/test_workload.h: http://gerrit.cloudera.org:8080/#/c/17326/1/src/kudu/integration-tests/test_workload.h@85 PS1, Line 85: set_begin_txn() nit: maybe, enforce this invariant with CHECK()? Another nit: it seems it's legit to call set_begin_txn(false) even if set_txn_id() has been called http://gerrit.cloudera.org:8080/#/c/17326/1/src/kudu/integration-tests/test_workload.h@87 PS1, Line 87: int64_t txn_id Since TestWorkload is most commonly used in more 'integrated' test scenarios, does it make sense to switch to using transaction tokens, like it is in Kudu C++ client API? http://gerrit.cloudera.org:8080/#/c/17326/1/src/kudu/integration-tests/test_workload.h@99 PS1, Line 99: // Commit the transaction that this workload is a part of upon calling : // StopAndJoin(). If set, either set_begin_txn() or set_txn_id() must be set : // as well. nit: it would be nice to explicitly mention what happens if set_commit_txn() isn't set but either begin_txn_ or txn_id_ is set. http://gerrit.cloudera.org:8080/#/c/17326/1/src/kudu/integration-tests/test_workload.h@102 PS1, Line 102: set_commit_txn Does it make sense to add a method to roll back the transaction explicitly in StopAndJoin(), not waiting for the backend to automatically roll it back once TestWorkload object goes out of scope? http://gerrit.cloudera.org:8080/#/c/17326/1/src/kudu/integration-tests/test_workload.cc File src/kudu/integration-tests/test_workload.cc: http://gerrit.cloudera.org:8080/#/c/17326/1/src/kudu/integration-tests/test_workload.cc@265 PS1, Line 265: // XXX(awong): this probably breaks. Yup, it seems txn-enabled workload limits the available set of row test patterns. Maybe, there should be some sort of sanity check for that somewhere in TestWorkload()::Setup()? -- To view, visit http://gerrit.cloudera.org:8080/17326 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia81daac8fcfd552603a0302c3d9aa411ea082ab1 Gerrit-Change-Number: 17326 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 21 Apr 2021 06:48:55 +0000 Gerrit-HasComments: Yes
