Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16699 )
Change subject: KUDU-2612: fuzz transactional inserts ...................................................................... Patch Set 3: (12 comments) I did a quick look over, but I don't think I understand what's going on here. Will try to get back if I have some time. 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@189 PS3, Line 189: -1 nit: maybe, introduce a constant for this, like kNoTxnId ? http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@441 PS3, Line 441: % 2 nit: why to change this? :) (just curious) http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@786 PS3, Line 786: valid What is 'valid' criteria here? Could you add more colors here? http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@868 PS3, Line 868: txn_id Is it acceptable to have txn_id == -1 here? If not, add a CHECK here? http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@925 PS3, Line 925: exists does not exist ? http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@950 PS3, Line 950: exists nit: does not exist? http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@1137 PS3, Line 1137: vector<int>{} nit: would {} suffice here? http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@1139 PS3, Line 1139: case TEST_COMMIT_TXN: nit: could you add a comment reminding that currently only INSERT can be transactional? http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@1142 PS3, Line 1142: exists[r] = true; Should there be a check like at line 1107 before assigning setting the exists[r] to 'true', something like CHECK(!exists[r]) http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@1147 PS3, Line 1147: EraseKeyReturnValuePtr(&pending_rows_per_txn, test_op.val); Does it make sense to verify that the key was present in the container? http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@1203 PS3, Line 1203: optional<ExpectedKeyValueRow> nit: might this be 'auto' ? http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@1695 PS3, Line 1695: nit: remove this extra empty row? -- 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: 3 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: Wed, 02 Dec 2020 05:37:18 +0000 Gerrit-HasComments: Yes
