Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10293 )
Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck ...................................................................... Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck-test.cc@477 PS4, Line 477: EXTRACT_ARRAY_CHECK_SIZE(r, cstate, "vote > I agree that adding a regression test wouldn't hurt here, but I'm not sure Hrm, I can't think of a great way; a somewhat naive but targeted unit test would be to create a RemoteError for the summary and ensure it doesn't get logged as WRONG_SERVER_UUID, though I'm not sure how useful that would be. I'm fine leaving the tests as they are. http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck.h File src/kudu/tools/ksck.h: http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck.h@276 PS4, Line 276: > Andrew, it does now, thanks for the tip. Also, nullptr must not be nullptr Looking good! Thanks for making that change http://gerrit.cloudera.org:8080/#/c/10293/8/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/10293/8/src/kudu/tools/ksck.cc@296 PS8, Line 296: ts->FetchInfo(&health).AndThen([&ts]() { : if (FLAGS_consensus) { : return ts->FetchConsensusState(); Ah, looking here, I don't think `health` gets set if `FetchConsensusState()` returns an error, so if it fails, we might end up with a bad server, but it would be considered HEALTHY, which I don't think seems right? In which case, maybe we should _also_ update `FetchConsensusState()` to `FetchConsensusState(&health)`? Or set the health in the not-OK to UNAVAILABLE if it's still HEALTHY? http://gerrit.cloudera.org:8080/#/c/10293/8/src/kudu/tools/ksck_remote.h File src/kudu/tools/ksck_remote.h: http://gerrit.cloudera.org:8080/#/c/10293/8/src/kudu/tools/ksck_remote.h@28 PS8, Line 28: #include "kudu/tools/ksck_results.h" We should be able to forward declare KsckServerHealth, no? -- To view, visit http://gerrit.cloudera.org:8080/10293 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f Gerrit-Change-Number: 10293 Gerrit-PatchSet: 8 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Wed, 16 May 2018 00:15:30 +0000 Gerrit-HasComments: Yes
