Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16699 )
Change subject: KUDU-2612: fuzz transactional inserts ...................................................................... Patch Set 4: (9 comments) http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc File src/kudu/integration-tests/fuzz-itest.cc: http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@169 PS4, Line 169: constexpr const int kNoTxnId = -1; nit: Maybe `kNoVal` since not all ops use this value as a txn id. http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@277 PS4, Line 277: const vector<TestOpType> kPkOnlyOps {TEST_INSERT_PK_ONLY, Any reason not to test transaction related ops here? http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@787 PS4, Line 787: // Generate a random valid sequence of operations for use as a fuzz test, i.e. More documentation. Awesome! http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@949 PS4, Line 949: if (!txn_operating_on_row && !exists[row_key]) { Dos this completely skip UPDATE_IGNORE ops for missing rows? http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@978 PS4, Line 978: if (!txn_operating_on_row && !exists[row_key]) { Dos this completely skip DELETE_IGNORE ops for missing rows? http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@992 PS4, Line 992: if (!ContainsKey(txns_needing_session_flush, txn_id)) continue; Nit: Add a comment explaining this continue like the others. http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@1075 PS4, Line 1075: if (pending_existence_per_txn.size() == 1 + FLAGS_max_open_txns) continue; Nit: Add a comment explaining this continue like the others. http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@1154 PS4, Line 1154: case TEST_UPSERT: Mind adding a TODO for the future about handling UPSERT pending_rows_per_txn when transactions supports UPSERTS? The same for UPDATE and DELETE below. http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@1177 PS4, Line 1177: // INSERT_IGNORE operation. Would it be worth also storing the OP type in the pending_rows_per_txn too so we can check this? I suspect that would be useful in the future for handling UPSERT/UPDATE/DELETE too. -- To view, visit http://gerrit.cloudera.org:8080/16699 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I719d42327ab18fda874332c9d6e1ae34aca8e846 Gerrit-Change-Number: 16699 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 08 Dec 2020 16:29:59 +0000 Gerrit-HasComments: Yes
