You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Will Berkeley (Code Review)" <ge...@cloudera.org> on 2016/06/27 03:26:52 UTC

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

Will Berkeley has uploaded a new change for review.

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

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), and documents that the numbers are not meaningful except in the leader
case.

Ideally, the metric wouldn't be exposed at all for non-leaders, because it is wrong, but I don't
think there is a good method to say "give me all the metrics except X, Y, Z" at present, or to
register and unregister metrics based on changes in runtime state.

As a bonus, the tablet's Raft role is now included on its /tablet page, and I fixed a typo in
the web ui.

Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/tserver/tserver-path-handlers.cc
3 files changed, 7 insertions(+), 5 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>

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

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

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

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

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

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

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

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

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

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.

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


Patch Set 8: -Verified

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

-- 
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: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

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

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon 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?


Line 827:   queue_->SetNonLeaderMode();
oh, I see, you change it to non-leader. perhaps just update the comment above a bit


Line 851:   // Committed index should be the same.
I'm not sure why this patch is changing the committed index metric to not advance on followers. It should be available (the leader propagates it to the followers), right? And I think it's useful to see if a follower got disconnected for some period of time (you'll see the committed index not advancing)


-- 
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 <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

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

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

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

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

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

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.

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


Patch Set 8: -Verified

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

-- 
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: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

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

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

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


Patch Set 8: Code-Review+1

Looks good to me. Mike, any comments on this?

-- 
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: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

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

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

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

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 << "<pre>" << EscapeForHtmlToString(state_->ToString()) << "</pre>" << std::endl;
This is a good thing to add but I think it wouldn't hurt to have both here. Something like:

 out << "<pre>"
     << EscapeForHtmlToString(state_->ToString())
     << EscapeForHtmlToString(queue_->ToString())
     << "</pre>" << 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 <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

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

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

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


Patch Set 8: Code-Review+2

-- 
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: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

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

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

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

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

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

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 << "<pre>" << EscapeForHtmlToString(state_->ToString()) << "</pre>" << 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 <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

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

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

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

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.

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


Patch Set 1:

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

-- 
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: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

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

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.

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


KUDU-763 consensus queue metrics on followers are messed up

On follower tablet replicas, the majority_done_ops and
in_progress_ops metrics are wrong.

majority_done_ops = committed_index - all_replicated_opid
in_progress_ops = last_appended - committed_index

There are two reasons why:

1) followers do not update their consensus queue's committed index
2) followers do not maintain a correct value for all_replicated_opid,
since their queues generally only track the local peer and the leader
does not notify followers when ops are all-replicated.

This patch fixes 1 by having consensus notify the follower queues of
the updated committed index when the consensus committed index is
updated. This makes in_progress_ops meaningful for followers. Note
that a follower queue's committed index is not used for anything
besides the metrics.

Fixing 2 would require having the leader notify followers when
operations are all-replicated. This isn't needed for consensus, and
would be used by the followers just for the majority_done_ops metric,
so I think it's best just to zero the metric for followers and
document that it is not meaningful in that case.

Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Reviewed-on: http://gerrit.cloudera.org:8080/3501
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
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, 54 insertions(+), 9 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

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

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 (#4).

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, 81 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/3501/4
-- 
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

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

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 (#7).

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

KUDU-763 consensus queue metrics on followers are messed up

On follower tablet replicas, the majority_done_ops and
in_progress_ops metrics are wrong.

majority_done_ops = committed_index - all_replicated_opid
in_progress_ops = last_appended - committed_index

There are two reasons why:

1) followers do not update their consensus queue's committed index
2) followers do not maintain a correct value for all_replicated_opid,
since their queues generally only track the local peer and the leader
does not notify followers when ops are all-replicated.

This patch fixes 1 by having consensus notify the follower queues of
the updated committed index when the consensus committed index is
updated. This makes in_progress_ops meaningful for followers. Note
that a follower queue's committed index is not used for anything
besides the metrics.

Fixing 2 would require having the leader notify followers when
operations are all-replicated. This isn't needed for consensus, and
would be used by the followers just for the majority_done_ops metric,
so I think it's best just to zero the metric for followers and
document that it is not meaningful in that case.

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, 54 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/3501/7
-- 
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: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

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

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 (#8).

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

KUDU-763 consensus queue metrics on followers are messed up

On follower tablet replicas, the majority_done_ops and
in_progress_ops metrics are wrong.

majority_done_ops = committed_index - all_replicated_opid
in_progress_ops = last_appended - committed_index

There are two reasons why:

1) followers do not update their consensus queue's committed index
2) followers do not maintain a correct value for all_replicated_opid,
since their queues generally only track the local peer and the leader
does not notify followers when ops are all-replicated.

This patch fixes 1 by having consensus notify the follower queues of
the updated committed index when the consensus committed index is
updated. This makes in_progress_ops meaningful for followers. Note
that a follower queue's committed index is not used for anything
besides the metrics.

Fixing 2 would require having the leader notify followers when
operations are all-replicated. This isn't needed for consensus, and
would be used by the followers just for the majority_done_ops metric,
so I think it's best just to zero the metric for followers and
document that it is not meaningful in that case.

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, 54 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/3501/8
-- 
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: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

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

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.

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


Patch Set 7:

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

-- 
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: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

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

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

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

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.

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


Patch Set 8:

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

-- 
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: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

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

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 (#2).

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), and documents that the numbers are not meaningful
except in the leader case.

Ideally, the metric wouldn't be exposed at all for non-leaders, because it is
wrong, but I don't think there is a good method to say "give me all the
metrics except X, Y, Z" at present, or to register and unregister metrics
based on changes in runtime state.

As a bonus, the tablet's Raft role is now included on its /tablet page, I
fixed a typo in the web ui, the table name is on the /tablet page, and the
(RPC) port number is displayed with the ts hostname in a couple of tables (I
run multiple ts on my MBP and it helps me...may help others while developing
but prob not important for "real" deployments).

Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/master/master-path-handlers.cc
M src/kudu/tserver/tserver-path-handlers.cc
4 files changed, 15 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/3501/2
-- 
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins

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

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No