David Ribeiro Alves has posted comments on this change. Change subject: Add integration tests for replay cache with writes ......................................................................
Patch Set 29: (8 comments) 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. http://gerrit.cloudera.org:8080/#/c/3519/24/src/kudu/integration-tests/raft_consensus-itest.cc 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 Done 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 @ " << address.ToString(); > how about append the last status as well? Done Line 1181: TEST_F(RaftConsensusITest, TestWritesWithExactlyOnceSemanticsWithChurnyElections) { > Needs comment Done 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. http://gerrit.cloudera.org:8080/#/c/3519/29/src/kudu/rpc/rpc_header.proto 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
