Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16699 )
Change subject: KUDU-2612: fuzz transactional inserts ...................................................................... Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc File src/kudu/integration-tests/fuzz-itest.cc: http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@441 PS3, Line 441: ey; > I just found it easier to read since I'm better versed at math operations t Makes sense :) http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@786 PS3, Line 786: > Done Thank you for adding the explanation! http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@868 PS3, Line 868: > It is acceptable, e.g. if we have chosen to insert not as a part of a trans Indeed -- I realized that after reading further, but it seems I forgot to discard that comment. Thank you for the confirmation! http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@1142 PS3, Line 1142: LLTHROUGH_INTENDE > Done Not sure I see that CHECK() was added? http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@1147 PS3, Line 1147: exists[test_op.val] = true; > Does it make sense to verify that the key was present in the container? Not sure whether you missed this one or this doesn't make much sense? 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@277 PS4, Line 277: const vector<TestOpType> kPkOnlyOps {TEST_INSERT_PK_ONLY, > Any reason not to test transaction related ops here? +1 http://gerrit.cloudera.org:8080/#/c/16699/4/src/kudu/integration-tests/fuzz-itest.cc@1734 PS4, Line 1734: {TEST_BEGIN_TXN, 0}, : {TEST_INSERT_IGNORE, 1, 0}, : {TEST_FLUSH_OPS, 0}, : {TEST_COMMIT_TXN, 0}, : : {TEST_INSERT_PK_ONLY, 0, -1}, : {TEST_INSERT_IGNORE_PK_ONLY, 0, -1}, : {TEST_DELETE, 0}, : {TEST_FLUSH_OPS, -1}, Does it make sense to add TEST_UPDATE and MINOR_COMPACT_DELTAS into the sequence, where UPDATE is on the affected row after committing the transaction? Or that deserves its own fuzz sequence? -- 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: Fri, 11 Dec 2020 07:28:46 +0000 Gerrit-HasComments: Yes
