[kudu-CR] KUDU-763 consensus queue metrics on followers are messed up

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: KUDU-763 consensus queue metrics on followers are messed up
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3501/6/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

Line 1774:   out << "" << EscapeForHtmlToString(state_->ToString()) << 
"" << std::endl;
> This is a good thing to add but I think it wouldn't hurt to have both here.
Also, maybe this change should be included in the other patch with the web UI 
changes


-- 
To view, visit http://gerrit.cloudera.org:8080/3501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-763 consensus queue metrics on followers are messed up

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: KUDU-763 consensus queue metrics on followers are messed up
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3501/5/src/kudu/consensus/consensus_queue-test.cc
File src/kudu/consensus/consensus_queue-test.cc:

Line 851:   // Committed index should be the same.
> This patch doesn't change how followers handle the committed index. It chan
I agree that since the committed index is sent to the followers by the leader, 
in theory we should be able to support it as a metric on the followers. I 
haven't really tried to dig into this but if we're not just fixing that problem 
then I would like to understand why not.


http://gerrit.cloudera.org:8080/#/c/3501/6/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

Line 1774:   out << "" << EscapeForHtmlToString(state_->ToString()) << 
"" << std::endl;
This is a good thing to add but I think it wouldn't hurt to have both here. 
Something like:

 out << ""
 << EscapeForHtmlToString(state_->ToString())
 << EscapeForHtmlToString(queue_->ToString())
 << "" << std::endl;


-- 
To view, visit http://gerrit.cloudera.org:8080/3501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-763 consensus queue metrics on followers are messed up

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

Change subject: KUDU-763 consensus queue metrics on followers are messed up
..


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/3501/5/src/kudu/consensus/consensus_queue-test.cc
File src/kudu/consensus/consensus_queue-test.cc:

PS5, Line 822: as a followe
> this seems to conflict with the 'SetLeaderMode' call below?
See next comment.


Line 827:   queue_->SetNonLeaderMode();
> oh, I see, you change it to non-leader. perhaps just update the comment abo
Done. Is this a reasonable way to do this? I just frankenstein'd the test from 
other tests.


Line 851:   // Committed index should be the same.
> I'm not sure why this patch is changing the committed index metric to not a
This patch doesn't change how followers handle the committed index. It changes 
how followers update metrics. The situation before this patch (and after) is 
that followers don't update the committed index-- I tested this a little bit to 
try to confirm it was what was leading to the bad metrics. I'm checking it's 
not updated in the test because if the committed index ever starts to be 
updated I'd like this test to fail, so somebody will take a look at how the 
change to the committed index code affects how metrics should work (e.g. maybe 
the follower could have the same metrics as leader at that point).


-- 
To view, visit http://gerrit.cloudera.org:8080/3501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-763 consensus queue metrics on followers are messed up

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-763 consensus queue metrics on followers are messed up
..


Patch Set 6:

Build Started http://104.196.14.100/job/kudu-gerrit/2261/

-- 
To view, visit http://gerrit.cloudera.org:8080/3501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] KUDU-763 consensus queue metrics on followers are messed up

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/3501

to look at the new patch set (#6).

Change subject: KUDU-763 consensus queue metrics on followers are messed up
..

KUDU-763 consensus queue metrics on followers are messed up

On non-leader tablet replicas, the majority_done_ops and
in_progress_ops metrics are wrong. The reason is that non-leaders do
not update their consensus queue's committed index
(queue_state_.committed_index). This patch hides these incorrect
numbers in the web ui, where they were previously exposed (I think
unintentionally, the code having printed the queue when it meant to
print the state), documents that the numbers are not meaningful
except in the leader case, and forces them to always be zero
whenever the tablet replica is not a leader.

Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
---
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
M src/kudu/consensus/raft_consensus.cc
4 files changed, 68 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/3501/6
-- 
To view, visit http://gerrit.cloudera.org:8080/3501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-763 consensus queue metrics on followers are messed up

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-763 consensus queue metrics on followers are messed up
..


Patch Set 5:

Build Started http://104.196.14.100/job/kudu-gerrit/2196/

-- 
To view, visit http://gerrit.cloudera.org:8080/3501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] KUDU-763 consensus queue metrics on followers are messed up

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/3501

to look at the new patch set (#5).

Change subject: KUDU-763 consensus queue metrics on followers are messed up
..

KUDU-763 consensus queue metrics on followers are messed up

On non-leader tablet replicas, the majority_done_ops and
in_progress_ops metrics are wrong. The reason is that non-leaders do
not update their consensus queue's committed index
(queue_state_.committed_index). This patch hides these incorrect
numbers in the web ui, where they were previously exposed (I think
unintentionally, the code having printed the queue when it meant to
print the state), documents that the numbers are not meaningful
except in the leader case, and forces them to always be zero
whenever the tablet replica is not a leader.

Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
---
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
M src/kudu/consensus/raft_consensus.cc
4 files changed, 68 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/3501/5
-- 
To view, visit http://gerrit.cloudera.org:8080/3501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-763 consensus queue metrics on followers are messed up

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-763 consensus queue metrics on followers are messed up
..


Patch Set 4:

Build Started http://104.196.14.100/job/kudu-gerrit/2195/

-- 
To view, visit http://gerrit.cloudera.org:8080/3501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] KUDU-763 consensus queue metrics on followers are messed up

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

Change subject: KUDU-763 consensus queue metrics on followers are messed up
..


Patch Set 3:

(2 comments)

> did you see my note about a test?

Whoops. I'll add one soon.

http://gerrit.cloudera.org:8080/#/c/3501/2/src/kudu/consensus/consensus_queue.cc
File src/kudu/consensus/consensus_queue.cc:

Line 77:   "not all peers. Not meaningful for 
non-leader tablet replicas.");
> I do think we should actually make the data at least be "0" in the case tha
Done


http://gerrit.cloudera.org:8080/#/c/3501/2/src/kudu/tserver/tserver-path-handlers.cc
File src/kudu/tserver/tserver-path-handlers.cc:

Line 251: peer.has_last_known_addr() ? peer.last_known_addr().host() : 
peer.permanent_uuid();
> would be good to move these web ui improvements to a separate patch
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/3501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-763 consensus queue metrics on followers are messed up

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-763 consensus queue metrics on followers are messed up
..


Patch Set 3:

did you see my note about a test?

-- 
To view, visit http://gerrit.cloudera.org:8080/3501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-763 consensus queue metrics on followers are messed up

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-763 consensus queue metrics on followers are messed up
..


Patch Set 3:

Build Started http://104.196.14.100/job/kudu-gerrit/2147/

-- 
To view, visit http://gerrit.cloudera.org:8080/3501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-763 consensus queue metrics on followers are messed up

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/3501

to look at the new patch set (#3).

Change subject: KUDU-763 consensus queue metrics on followers are messed up
..

KUDU-763 consensus queue metrics on followers are messed up

On non-leader tablet replicas, the majority_done_ops and in_progress_ops
metrics are wrong. The reason is that non-leaders do not update their
consensus queue's committed index (queue_state_.committed_index). This patch
hides these incorrect numbers in the web ui, where they were previously
exposed (I think unintentionally, the code having printed the queue when it
meant to print the state), documents that the numbers are not meaningful
except in the leader case, and forces them to always be zero whenever the
tablet replica is not a leader.

Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/raft_consensus.cc
2 files changed, 10 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/3501/3
-- 
To view, visit http://gerrit.cloudera.org:8080/3501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-763 consensus queue metrics on followers are messed up

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-763 consensus queue metrics on followers are messed up
..


Patch Set 2:

(2 comments)

Should have a test as well for this

http://gerrit.cloudera.org:8080/#/c/3501/2/src/kudu/consensus/consensus_queue.cc
File src/kudu/consensus/consensus_queue.cc:

Line 77:   "not all peers. Not meaningful for 
non-leader tablet replicas.");
I do think we should actually make the data at least be "0" in the case that 
it's a follower. Right now, the numbers go negative which is pretty odd. 
Remember that the metrics aren't just exposed on that web UI, but also in other 
systems which capture metrics.


http://gerrit.cloudera.org:8080/#/c/3501/2/src/kudu/tserver/tserver-path-handlers.cc
File src/kudu/tserver/tserver-path-handlers.cc:

Line 251: peer.has_last_known_addr() ? Substitute("$0:$1",
would be good to move these web ui improvements to a separate patch


-- 
To view, visit http://gerrit.cloudera.org:8080/3501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-763 consensus queue metrics on followers are messed up

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-763 consensus queue metrics on followers are messed up
..


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/2083/

-- 
To view, visit http://gerrit.cloudera.org:8080/3501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No