Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16699 )
Change subject: KUDU-2612: fuzz transactional inserts ...................................................................... Patch Set 5: (10 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: "TEST_ABORT_TXN", > nit: Maybe `kNoVal` since not all ops use this value as a txn id. Done http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@277 PS4, Line 277: TEST_RESTART_TS, > Any reason not to test transaction related ops here? Done http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@787 PS4, Line 787: case TEST_INSERT_IGNORE: > More documentation. Awesome! Yea, jumping into this test without much context is pretty intimidating. Hope it helps! http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@949 PS4, Line 949: } > Dos this completely skip UPDATE_IGNORE ops for missing rows? Good catch! Totally borked the UPDATE_IGNORE path and some of the checking for setting data_in_dms. http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@978 PS4, Line 978: // The row is being operated on by an in-flight op, but the pending > Dos this completely skip DELETE_IGNORE ops for missing rows? Done http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@992 PS4, Line 992: } > Nit: Add a comment explaining this continue like the others. Done http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@1075 PS4, Line 1075: // this -- they only set 'data_in_mrs' once committed. > Nit: Add a comment explaining this continue like the others. Done http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@1134 PS4, Line 1134: } > warning: method 'ValidateFuzzCase' can be made static [readability-convert- Done http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@1154 PS4, Line 1154: ops->emplace_back(TEST_FLUSH_OPS, txn_id); > Mind adding a TODO for the future about handling UPSERT pending_rows_per_tx Done http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@1177 PS4, Line 1177: auto pending_existence = EraseKeyReturnValuePtr(&pending_existence_per_txn, txn_id); > Would it be worth also storing the OP type in the pending_rows_per_txn too Done -- 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: 5 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 11 Dec 2020 07:15:29 +0000 Gerrit-HasComments: Yes
