Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9787 )
Change subject: KUDU-2364 Add extra check in ksck for tserver ID ...................................................................... Patch Set 2: (3 comments) This also needs a test, maybe using the mock tablet server. http://gerrit.cloudera.org:8080/#/c/9787/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9787/2//COMMIT_MSG@21 PS2, Line 21: Fetched info from 1 Tablet Servers, 1 weren't reachable We'll need a more specific error message for the uuid mismatch case, and it'll also need to appear at the bottom with the summary information. http://gerrit.cloudera.org:8080/#/c/9787/2/src/kudu/tools/ksck_remote.cc File src/kudu/tools/ksck_remote.cc: http://gerrit.cloudera.org:8080/#/c/9787/2/src/kudu/tools/ksck_remote.cc@116 PS2, Line 116: req.set_dest_uuid(uuid()); > Couldn't we do the UUID verification client-side by issuing an additional G I would argue for checking the UUID via GetStatus(). I think it makes sense to have the Ping call just above this one be replaced with GetStatus, for the following reasons: 1. It's a clean separation between two different things: getting the tablets and getting server info. We might still want to get the tablet information even if the uuids mismatch, for example. 2. GetStatus() has other info we could use, for example version information. It'd be nice for ksck to check for version heterogeneity (but I'm not asking for that in this patch). I think it might be good, in general, for client to be able to set a destination uuid and have servers check it, so I do agree with Adar that there's nothing wrong with extending ListTablets as you have. But for a ksck uuid check I think GetStatus is most appropriate. http://gerrit.cloudera.org:8080/#/c/9787/2/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/9787/2/src/kudu/tserver/tablet_service.cc@1366 PS2, Line 1366: \ Extra \? -- To view, visit http://gerrit.cloudera.org:8080/9787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia2c18ba7af8eaa6f5e4d7842f18754d2c1e32526 Gerrit-Change-Number: 9787 Gerrit-PatchSet: 2 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Mon, 26 Mar 2018 00:45:04 +0000 Gerrit-HasComments: Yes
