Mike Percy has posted comments on this change. Change subject: [consensus] Add consensus op-level lag metrics ......................................................................
Patch Set 12: (4 comments) http://gerrit.cloudera.org:8080/#/c/6451/10/src/kudu/consensus/consensus_queue-test.cc File src/kudu/consensus/consensus_queue-test.cc: Line 253: page_size_estimator.set_committed_index(0); > The key thing is that in this and only this test, the size of the protobuf Thanks for the explanation. Makes sense. http://gerrit.cloudera.org:8080/#/c/6451/12/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: PS12, Line 602: queue_state_.last_idx_appended_to_leader - queue_state_.last_appended.index() Just thought of one last thing. I think it is possible that the leader tells us it has truncated the log but we are under memory pressure and rejecting requests. This will cause the follower to be ahead of the leader which will result in a negative lag, which possibly could throw off averages tracked by monitoring systems etc. So we may want to set a floor of 0 for this metric and make this line: std::min<uint64_t>(0, queue_state_.last_idx_appended_to_leader - queue_state_.last_appended.index()) http://gerrit.cloudera.org:8080/#/c/6451/10/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: Line 1152: queue_->UpdateLastIndexAppendedToLeader(request->last_idx_appended_to_leader()); > As discussed there is a _default_ default that should be fine. Yeah, I forgot there is a default default. :) Sounds good. http://gerrit.cloudera.org:8080/#/c/6451/12/src/kudu/consensus/raft_consensus_quorum-test.cc File src/kudu/consensus/raft_consensus_quorum-test.cc: Line 981: ASSERT_EQ(follower->queue_->metrics_.num_ops_behind_leader->value(), 0); nit: the order should be ASSERT_EQ(expected, actual) in order to get a non-confusing error message from gtest if it fails. -- 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: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
