Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9781 )
Change subject: consensus: Expose health status in GetConsensusState() RPC ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/9781/1/src/kudu/consensus/consensus.proto File src/kudu/consensus/consensus.proto: http://gerrit.cloudera.org:8080/#/c/9781/1/src/kudu/consensus/consensus.proto@479 PS1, Line 479: UNSPECIFIED_HEALTH_REPORT = 0; : EXCLUDE_HEALTH_REPORT = 1; : INCLUDE_HEALTH_REPORT = 2; Why not just UNSPECIFIED (or UNKNOWN) EXCLUDE INCLUDE ? I think my question is: why do we need the prefix? >From the other side, maybe enum HealthInfoOptions { UNKNOWN = 0; INCLUDE_HEALTH_REPORT, EXCLUDE_HEALTH_REPORT, }; http://gerrit.cloudera.org:8080/#/c/9781/1/src/kudu/consensus/consensus.proto@482 PS1, Line 482: ; nit: drop the semicolon? http://gerrit.cloudera.org:8080/#/c/9781/1/src/kudu/consensus/consensus.proto@493 PS1, Line 493: include_health_report is set to true This seems to be outdated: now it's a enumeration, right? http://gerrit.cloudera.org:8080/#/c/9781/1/src/kudu/consensus/consensus.proto@495 PS1, Line 495: IncludeHealthReport Maybe (if we ever need to extend this in the future), it's worth to add some structure like ResultDetailOptions that now will contain the health report options and would aggregate further options, if needed? -- To view, visit http://gerrit.cloudera.org:8080/9781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I15b8931156183e1713b902d22abbc80ca393f6b9 Gerrit-Change-Number: 9781 Gerrit-PatchSet: 1 Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Fri, 23 Mar 2018 21:58:53 +0000 Gerrit-HasComments: Yes
