Mike Percy 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 This is a namespace-level enum so that would create consensus::UNKNOWN, etc, which seems overly broad and prone to naming conflicts. http://gerrit.cloudera.org:8080/#/c/9781/1/src/kudu/consensus/consensus.proto@482 PS1, Line 482: ; > nit: drop the semicolon? ah, force of habit 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? Ah, thanks for the catch. 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 som Can you think of a scenario where we will need that? If not, it seems reasonable to add an additional parameter as a sibling of this one if we ever need it. -- 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-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Fri, 23 Mar 2018 22:22:35 +0000 Gerrit-HasComments: Yes
