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.
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
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,
> 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.
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-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>