David Ribeiro Alves has posted comments on this change.

Change subject: Add integration tests for replay cache with writes

Patch Set 29:


i moved these to a new itest, also moved a couple of methods out of raft 
consensus itest to the parent class so that I can use them in the new test.

File src/kudu/integration-tests/raft_consensus-itest.cc:

PS24, Line 1043: RestartAnyCrashedTabletServers()
> This method needs a flag indicating whether servers are allowed to crash in
good catch. added the flag and also added an AssertNoTabletServersCrashed

Line 1047:   bool mismatched = false;
> Please add a comment here to summarize the purpose of this logic. It took m

PS24, Line 1079: (1234
> s/1234/SeedRandom()/
this is on purpose so that all threads are seeded with the same random as they 
are supposed to generate the same rows. Will add a comment though

Line 1125:         status = controller.status();
> Returning controller.status() is already what Write() does. See proxy.cc L9
Was mimicking the client code, but writes are async there. you're right. Done

Line 1141:         FAIL() << "Couldn't write request to tablet server @ " << 
> how about append the last status as well?

Line 1181: TEST_F(RaftConsensusITest, 
TestWritesWithExactlyOnceSemanticsWithChurnyElections) {
> Needs comment

PS24, Line 1184:  defined(__SANITIZE_ADDRESS__) || defined(ADDRESS_SANITIZER)
> we should only need one of these two, right?
gcc calls it one thing and clang calls it another. We've used the two elsewhere.

File src/kudu/rpc/rpc_header.proto:

Line 130:   required int64 attempt_no = 4;
> Please squash this change into a previous commit, or squash this whole patc
this change is first needed in this patch. Usually attempt numbers never get 
this high, its just that the tests used very high numbers to make sure they are 
unique. Will mention this in the commit message.

To view, visit http://gerrit.cloudera.org:8080/3519
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I35722eb1c83f97e886cfe9d6b03ed95bcd62429f
Gerrit-PatchSet: 29
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to