You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2020/12/02 05:37:18 UTC
[kudu-CR] KUDU-2612: fuzz transactional inserts
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 <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 02 Dec 2020 05:37:18 +0000
Gerrit-HasComments: Yes