Will Berkeley 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 2: (5 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@101 PS4, Line 101: 345; Just for code hygiene, could you add a CHECK(health) here to ensure health is not nullptr? Alternatively, the function could not set the output parameter if nullptr is passed. http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck-test.cc@129 PS4, Line 129: nit: "these variables". 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 semantics are in that case (probably, it is just not set as an output param in that case). 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: return Status::UuidMismatch(Substitute("ID reported by tablet server ($0) doesn't " > thanks for the tip. as for the masters, it's a good idea, but I'd rather do We have an expected uuid known ahead of time, from an authority, for each tserver- the uuid known to the leader master. We do not have authoritative uuids for the masters, just the addresses from the command line, so it's tricker to claim a master has the wrong uuid. The test that comes to mind is that if a leader master expected UUID A but the master reports UUID B, it's an imposter. We can't do that check in FetchInfo. We also can't do it if there's no leader master. Agree the WRONG_UUID_LOGIC for masters should be another patch. 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@155 PS4, Line 155: Also need to handle the nullptr case. -- 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: 2 Gerrit-Owner: Attila Bukor <abu...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Thu, 10 May 2018 18:24:01 +0000 Gerrit-HasComments: Yes
