Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10300 )
Change subject: [tools] ksck imp. [8/n]: Easy-to-see errors when running w/o admin ...................................................................... Patch Set 5: Code-Review+2 (3 comments) http://gerrit.cloudera.org:8080/#/c/10300/3/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/10300/3/src/kudu/tools/ksck.cc@146 PS3, Line 146: s.IsRemoteError > These particular errors come back as RemoteError. OK, but there are also calls where a KuduClient would be used, and KuduClient converts RemoteError with specific PB error code into NotAuthorized. My thinking was to make this function applicable for those cases as well. However, I think that's OK in the YANGNI paradigm to postpone that up to the time when this function is about to be applied for statuses produced by KuduClient methods. http://gerrit.cloudera.org:8080/#/c/10300/3/src/kudu/tools/ksck_results.cc File src/kudu/tools/ksck_results.cc: http://gerrit.cloudera.org:8080/#/c/10300/3/src/kudu/tools/ksck_results.cc@171 PS3, Line 171: case KsckServerHealth::UNAUTHORIZED: : return 1; : case KsckServerHealth::UNAVAILABLE: : return 2; : case KsckServerHealth::WRONG_SERVER_UUID: > I consider WRONG_SERVER_UUID to be worse because it means there's a tablet Maybe, that's just another reason to separate that different statuses like UNAVAILABLE and WRONG_SERVER_UUID :) OK, if from server health perspective a dead server is better than alive server at wrong place, that's strange but acceptable. http://gerrit.cloudera.org:8080/#/c/10300/3/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/10300/3/src/kudu/tools/tool.proto@224 PS3, Line 224: UNAUTHORIZED = 3; > It's not UNKNOWN, it's UNAUTHORIZED. I agree ServerHealth has already grown Yes, I know. My question was about whether it would be enough in that case to report the server's health as UNKNOWN instead of proliferating detailed error cases. It seems there is some value of having the exact error specified, though. -- To view, visit http://gerrit.cloudera.org:8080/10300 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I644957103efefceeba4b82f4557376a603507423 Gerrit-Change-Number: 10300 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Fri, 25 May 2018 20:38:23 +0000 Gerrit-HasComments: Yes
