Attila Bukor has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10076 )

Change subject: KUDU-2287 Expose election failures as a metric
......................................................................


Patch Set 2:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/10076/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10076/1//COMMIT_MSG@7
PS1, Line 7: KUDU-2287 Expose election failures as a metric
> The JIRA suggests either this or an actual wall clock time since there was
I opted for the counter as it was more precise as we don't have "time since" 
metrics, so we could update the metric at each election failure, but not in 
between. Or is there a better approach to this that I didn't think of? I guess 
we could expose both numbers though.


http://gerrit.cloudera.org:8080/#/c/10076/1//COMMIT_MSG@9
PS1, Line 9: This patch exposes as a metric the number of election failures 
since the
           : last su
> nit: Could you rewrite this as complete sentence:
Done


http://gerrit.cloudera.org:8080/#/c/10076/1/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:

http://gerrit.cloudera.org:8080/#/c/10076/1/src/kudu/consensus/raft_consensus.h@839
PS1, Line 839: The number of times this node has called and lost a leader 
election
> This is different than what the patch is claiming the metric measures. fail
yep, the description of the metric wasn't clear I guess, but I believe this is 
what this metric should keep track of. I agree that each node will have a 
different number here. These numbers should be summarized on the client side if 
necessary (e.g. in ksck).


http://gerrit.cloudera.org:8080/#/c/10076/1/src/kudu/consensus/raft_consensus.h@855
PS1, Line 855: >>
> nit: While you're here, the space between the >'s isn't necessary in C++11.
fixed, wasn't sure if I should touch this line in this patch and didn't want to 
have two consecutive lines inconsistently formatted


http://gerrit.cloudera.org:8080/#/c/10076/1/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

http://gerrit.cloudera.org:8080/#/c/10076/1/src/kudu/consensus/raft_consensus.cc@156
PS1, Line 156: "Number of failed elections on this node since there was a s
> Not exactly correct according to my prev comment.
Done


http://gerrit.cloudera.org:8080/#/c/10076/1/src/kudu/consensus/raft_consensus.cc@157
PS1, Line 157: ion a
> resets
Done


http://gerrit.cloudera.org:8080/#/c/10076/1/src/kudu/consensus/raft_consensus.cc@2290
PS1, Line 2290: ed_elections_metric_->set_value(faile
> Do you mean failed_elections_metric_? It should be non-null because of the
yep, I meant failed_elections_metric_, removed it if unnecessary (raft_term_ 
was used similarly)


http://gerrit.cloudera.org:8080/#/c/10076/1/src/kudu/consensus/raft_consensus.cc@2453
PS1, Line 2453: f we called an election and one of th
> Same.
Done



--
To view, visit http://gerrit.cloudera.org:8080/10076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1b25df258cdba7bdae7bb2d7b4eb3d73b53425c3
Gerrit-Change-Number: 10076
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <abu...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <abu...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com>
Gerrit-Comment-Date: Tue, 17 Apr 2018 17:33:25 +0000
Gerrit-HasComments: Yes

Reply via email to