David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1506 Add consensus lag metrics ......................................................................
Patch Set 3: (8 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." 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 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 PS3, Line 86: num_ops_behind_max num_ops_behind_max_replicated 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()) 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? 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? PS3, Line 1047: queue_->UpdateLagMetrics(request->max_replicated_index()); I think it's fine to just reuse UpdateFollowerWatermarks with the extra argument. This has the extra advantage of only updating this for valid leaders (whereas here an invalid leader could move this index) -- 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: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
