Andrew Wong has posted comments on this change. Change subject: [consensus] Add consensus op-level lag metrics ......................................................................
Patch Set 10: (10 comments) http://gerrit.cloudera.org:8080/#/c/6451/10/src/kudu/consensus/consensus.proto File src/kudu/consensus/consensus.proto: Line 343: // The highest index that is known to be replicated by all members of > Ha, nice catch :) http://gerrit.cloudera.org:8080/#/c/6451/10/src/kudu/consensus/consensus_queue-test.cc File src/kudu/consensus/consensus_queue-test.cc: Line 247: // Note: This estimation must be precise in order to determine the exact number of messages that > This was a little hard to me to grok. Can you clarify in this message that Done Line 253: page_size_estimator.set_last_idx_appended_to_leader(0); > I'm not sure of the purpose of the changes to this test. Does this mean thi The key thing is that in this and only this test, the size of the protobuf must be the expected size of a request, since it uses it to set a flag that would otherwise be set by the user. http://gerrit.cloudera.org:8080/#/c/6451/10/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: PS10, Line 88: is > believes it is Done Line 156: queue_state_.last_idx_appended_to_leader = queue_state_.last_appended.index(); > Maybe we should not track this in leader mode and instead do this re-initia Hmm good observation that one of the reasons we track on LEADER is that if we don't, when we step down as leader, we could end up with a negative metric (and this can be avoided by moving this down to when we start as nonleader). PS10, Line 301: // If LEADER, update the index of the last appended operation. : queue_state_.last_idx_appended_to_leader = last_id.index(); > Seems strange to me to track this at all as the leader; see below for a sug Done PS10, Line 380: last_idx_appended_to_leader > last_appended.index() Done Line 603: queue_state_.last_idx_appended_to_leader = std::max({queue_state_.last_idx_appended_to_leader, > Based on the changes we have made, I don't think we want the max() anymore Hmm. So when processing the request, we can UpdateLastIndexAppendedToLeader and just UpdateLagMetrics when there's no new information from the leader. Makes sense. PS10, Line 605: metrics_.num_ops_behind_leader->set_value( : queue_state_.last_idx_appended_to_leader - queue_state_.last_appended.index()); > How about: Given the above change, makes sense. http://gerrit.cloudera.org:8080/#/c/6451/10/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: Line 1152: queue_->UpdateLagMetrics(request->last_idx_appended_to_leader()); > I think this will cause the field to be 'required' without a default being As discussed there is a _default_ default that 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: 10 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-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
