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
