Andrew Wong has posted comments on this change. Change subject: KUDU-871. Support tombstoned voting ......................................................................
Patch Set 10: (9 comments) http://gerrit.cloudera.org:8080/#/c/6960/10/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: PS10, Line 1452: boost:: nit: "using boost::optional" is already specified http://gerrit.cloudera.org:8080/#/c/6960/10/src/kudu/consensus/raft_consensus.h File src/kudu/consensus/raft_consensus.h: PS10, Line 351: | ^ : // | | : // +--------------------+ nit: prefer the slimmer version at L424 Or have this in one place and have SetStateUnlocked() refer here? http://gerrit.cloudera.org:8080/#/c/6960/10/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: PS10, Line 3044: back come nit: come back http://gerrit.cloudera.org:8080/#/c/6960/10/src/kudu/integration-tests/tombstoned_voting-itest.cc File src/kudu/integration-tests/tombstoned_voting-itest.cc: PS10, Line 46: using std::string; : using std::vector; nit: include these? http://gerrit.cloudera.org:8080/#/c/6960/10/src/kudu/integration-tests/tombstoned_voting-stress-test.cc File src/kudu/integration-tests/tombstoned_voting-stress-test.cc: PS10, Line 44: using std::atomic; : using std::string; : using std::thread; : using std::unique_lock; : using std::vector; nit: includes? http://gerrit.cloudera.org:8080/#/c/6960/10/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: PS10, Line 347: // TODO(mpercy): Set to failed if Init() fails? Does this still apply? http://gerrit.cloudera.org:8080/#/c/6960/10/src/kudu/tablet/tablet_metadata.h File src/kudu/tablet/tablet_metadata.h: PS10, Line 357: Has no meaning for non-tombstoned tablets Is this to say that this will be boost::none for non-tombstoned tablets? http://gerrit.cloudera.org:8080/#/c/6960/10/src/kudu/tablet/tablet_replica.h File src/kudu/tablet/tablet_replica.h: PS10, Line 199: SetError nit: sort of brought this up on Slack, I think the naming for this could be a bit clearer. SetError() makes it seem like a simple setter, but it's not. Maybe SetErrorAndShutdown()? Or SetFailed() and keep the FAILED state as we discussed. The latter has the added benefit of being an external indicator of failure, rather than having each external accessor (e.g. web UI, ksck, etc.) get the error_ member. http://gerrit.cloudera.org:8080/#/c/6960/10/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: PS10, Line 298: RETURN_NOT_OK(WriteConsensusMetadata()); This change isn't noted in the commit message. Mind documenting it, or at least adding a comment explaining? -- To view, visit http://gerrit.cloudera.org:8080/6960 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia19d75b185299443b27f41e468bbae20065e7570 Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Edward Fancher <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
