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 4: (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: TEST_F(KsckTest, TestWrongUUIDTabletServer) { Hmm.. maybe also add a regression test specific to KUDU-2426 that other remote errors don't end up with WRONG_SERVER_UUID? Also it'd be nice if we had an integration test that actually tested the RemoteTabletServer's behavior, maybe using the ClusterVerifier and a mini cluster? TestRestartWithDifferentUUID in raft_consensus-itest.cc might be worth checking out. 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: // Connects to the configured tablet server and populates the fields of this class. > Document if 'health' is allowed to be nullptr, and if it is, what the seman Does returning a non-OK status mean that health is guaranteed to not be HEALTHY? Similarly, if this returns OK, does that mean health is guaranteed to be HEALTHY? I think it'd be nice if both of these were true, although guaranteeing them might take some work. If not, I at least agree with Will, the new semantics of this function should be documented. http://gerrit.cloudera.org:8080/#/c/10293/2/src/kudu/tools/ksck_remote.cc File src/kudu/tools/ksck_remote.cc: http://gerrit.cloudera.org:8080/#/c/10293/2/src/kudu/tools/ksck_remote.cc@165 PS2, Line 165: if (response_uuid != uuid()) { > We have an expected uuid known ahead of time, from an authority, for each t Ah, I meant changing the signature of the KsckMaster::FetchInfo() to also return server health (ie UNAVAILABLE). But I agree, it belongs in a separate patch if anything. http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck_remote.cc File src/kudu/tools/ksck_remote.cc: http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck_remote.cc@166 PS4, Line 166: *health = KsckServerHealth::WRONG_SERVER_UUID; Why don't we return an error here anymore? -- 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: 4 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: Thu, 10 May 2018 22:19:18 +0000 Gerrit-HasComments: Yes
