Andrew Wong has posted comments on this change. Change subject: KUDU-1506 Add consensus lag metrics ......................................................................
Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/6451/3//COMMIT_MSG Commit Message: PS3, Line 9: behind > "... behind the leader followers are with regard to replicated operations." Done http://gerrit.cloudera.org:8080/#/c/6451/3/src/kudu/consensus/consensus.proto File src/kudu/consensus/consensus.proto: PS3, Line 359: peer > peer is Done http://gerrit.cloudera.org:8080/#/c/6451/3/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: PS3, Line 86: Operations behind max > ... max replicated Done PS3, Line 314: if (queue_state_.max_replicated_index < last_id.index()) { : queue_state_.max_replicated_index = last_id.index(); : } > use std::max(queue_state_.max_replicated_index, last_id.index()) Done http://gerrit.cloudera.org:8080/#/c/6451/3/src/kudu/consensus/consensus_queue.h File src/kudu/consensus/consensus_queue.h: PS3, Line 248: // Called by the concensus implementation to update the lag metrics : // before applying the contents of a request. Also used throughout consensus : // via UpdateMetrics() as operations are applied. : void UpdateLagMetrics(int64_t max_replicated_index); > why not add an argument to the method above? See other comment. http://gerrit.cloudera.org:8080/#/c/6451/3/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: PS3, Line 127: METRIC_DEFINE_gauge_int64(tablet, num_ops_behind_max, : "Operations Behind Max Index", : kudu::MetricUnit::kOperations, : "Number of operations behind the max replicated index received by and " : "reported by leader."); > wait why have the same metric twice? Done PS3, Line 1047: queue_->UpdateLagMetrics(request->max_replicated_index()); > I think it's fine to just reuse UpdateFollowerWatermarks with the extra arg I'd wanted to keep it separate since only the lag metric will be updated right after the request is received, whereas all metrics are updated after the transactions are committed i.e. UpdateFollowerWatermarks() gets called later on in this function and this UpdateLagMetrics(). In that sense, UpdateLagMetrics() and UpdateFollowerMetrics() would have slightly different use-cases. Good point about the leader checking though. An extra locking call for something with such a short critical path should be fine. -- To view, visit http://gerrit.cloudera.org:8080/6451 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ida8e992cc2397ca8d5873e62961a65f618d52c36 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
