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 <mpe...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Edward Fancher <e...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
