Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16699 )
Change subject: KUDU-2612: fuzz transactional inserts ...................................................................... Patch Set 4: (11 comments) Test failures are unrelated. 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: > nit: maybe, introduce a constant for this, like kNoTxnId ? Done http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@441 PS3, Line 441: ey; > nit: why to change this? :) (just curious) I just found it easier to read since I'm better versed at math operations than bit operations (and assume most are). They should compile to the same instructions though https://godbolt.org/z/jGvYxd http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@786 PS3, Line 786: > What is 'valid' criteria here? Could you add more colors here? Done http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@868 PS3, Line 868: > Is it acceptable to have txn_id == -1 here? If not, add a CHECK here? It is acceptable, e.g. if we have chosen to insert not as a part of a transaction. http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@925 PS3, Line 925: ouchin > does not exist ? Done http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@950 PS3, Line 950: The ro > nit: does not exist? Done http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@1137 PS3, Line 1137: > nit: would {} suffice here? Interestingly enough, no: ../../src/kudu/gutil/map-util.h:467:6: note: candidate template ignored: substitution failure [with Collection = std::__1::unordered_map<int, std::__1::vector<int, std::__1::allocator<int> >, std::__1::hash<int>, std::__1::equal_to<int>, std::__1::allocator<std::__1::pair<const int, std::__1::vector<int, std::__1::allocator<int> > > > >]: deduced incomplete pack <const int &, (no value)> for template parameter 'Args' http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@1139 PS3, Line 1139: case TEST_INSERT: > nit: could you add a comment reminding that currently only INSERT can be tr Done http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@1142 PS3, Line 1142: LLTHROUGH_INTENDE > Should there be a check like at line 1107 before assigning setting the exis Done http://gerrit.cloudera.org:8080/#/c/16699/3/src/kudu/integration-tests/fuzz-itest.cc@1203 PS3, Line 1203: ops that are current pending > nit: might this be 'auto' ? Done 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? 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: 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, 04 Dec 2020 06:58:44 +0000 Gerrit-HasComments: Yes
