You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2018/02/08 03:55:54 UTC

[kudu-CR] WIP: KUDU-2291 (part 2): Add a /stacks page 

Hello Will Berkeley, Mike Percy,

I'd like you to do a code review. Please visit

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

to review the following change.


Change subject: WIP: KUDU-2291 (part 2): Add a /stacks page
......................................................................

WIP: KUDU-2291 (part 2): Add a /stacks page

This adds a simple /stacks web page which dumps the currently running
threads in a plain text format.

Since we often have a lot of idle threadpool threads sitting around in
the "wait for work" state, the output collapses all threads with
matching stacks and only displays the stack once, making it more
suitable for human consumption.

Example output from a local kudu-master:
  https://gist.github.com/b64739ee5fb146ea1953380f57b996c4

WIP since maybe this should be consolidated with /threadz?

Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b
---
M src/kudu/server/default_path_handlers.cc
1 file changed, 88 insertions(+), 0 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b
Gerrit-Change-Number: 9253
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2291 (part 2): Add a /stacks page

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9253 )

Change subject: KUDU-2291 (part 2): Add a /stacks page
......................................................................


Patch Set 5: Verified+1

unrelated consensus test flke


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b
Gerrit-Change-Number: 9253
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 20 Feb 2018 20:22:25 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2291 (part 2): Add a /stacks page

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins, 

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

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

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

Change subject: KUDU-2291 (part 2): Add a /stacks page
......................................................................

KUDU-2291 (part 2): Add a /stacks page

This adds a simple /stacks web page which dumps the currently running
threads in a plain text format.

Since we often have a lot of idle threadpool threads sitting around in
the "wait for work" state, the output collapses all threads with
matching stacks and only displays the stack once, making it more
suitable for human consumption.

Example output from a local kudu-master:
  https://gist.github.com/b64739ee5fb146ea1953380f57b996c4

Longer term we may want to integrate stack-tracing capability into the
/threadz view as well, but for now I left this as a low-level utility
which doesn't access the thread manager, etc. I left a few TODOs for
further enhancements, but I've already found this helpful for
understanding some perf anomalies while playing with YCSB, so let's get
it committed and improve as we go.

Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b
---
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/server/default_path_handlers.cc
M src/kudu/util/debug-util.cc
3 files changed, 106 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/9253/4
-- 
To view, visit http://gerrit.cloudera.org:8080/9253
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b
Gerrit-Change-Number: 9253
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2291 (part 2): Add a /stacks page

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has removed Kudu Jenkins from this change.  ( http://gerrit.cloudera.org:8080/9253 )

Change subject: KUDU-2291 (part 2): Add a /stacks page
......................................................................


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/9253
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b
Gerrit-Change-Number: 9253
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2291 (part 2): Add a /stacks page

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins, 

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

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

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

Change subject: KUDU-2291 (part 2): Add a /stacks page
......................................................................

KUDU-2291 (part 2): Add a /stacks page

This adds a simple /stacks web page which dumps the currently running
threads in a plain text format.

Since we often have a lot of idle threadpool threads sitting around in
the "wait for work" state, the output collapses all threads with
matching stacks and only displays the stack once, making it more
suitable for human consumption.

Example output from a local kudu-master:
  https://gist.github.com/b64739ee5fb146ea1953380f57b996c4

Longer term we may want to integrate stack-tracing capability into the
/threadz view as well, but for now I left this as a low-level utility
which doesn't access the thread manager, etc. I left a few TODOs for
further enhancements, but I've already found this helpful for
understanding some perf anomalies while playing with YCSB, so let's get
it committed and improve as we go.

Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b
---
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/server/default_path_handlers.cc
2 files changed, 95 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b
Gerrit-Change-Number: 9253
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2291 (part 2): Add a /stacks page

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9253 )

Change subject: KUDU-2291 (part 2): Add a /stacks page
......................................................................


Patch Set 5: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b
Gerrit-Change-Number: 9253
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 21 Feb 2018 00:07:14 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2291 (part 2): Add a /stacks page

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9253 )

Change subject: KUDU-2291 (part 2): Add a /stacks page
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9253/3/src/kudu/integration-tests/linked_list-test-util.h
File src/kudu/integration-tests/linked_list-test-util.h:

http://gerrit.cloudera.org:8080/#/c/9253/3/src/kudu/integration-tests/linked_list-test-util.h@315
PS3, Line 315: master_pages.emplace_back("/metrics");
             :     master_pages.emplace_back("/masters");
             :     master_pages.emplace_back("/tables");
             :     master_pages.emplace_back("/dump-entities");
             :     master_pages.emplace_back("/tablet-servers");
             :     master_pages.emplace_back("/mem-trackers");
             :     master_pages.emplace_back("/stacks");
> :Puts Adar hat on: Could you sort these in alphabetical order?
Done


http://gerrit.cloudera.org:8080/#/c/9253/3/src/kudu/server/default_path_handlers.cc
File src/kudu/server/default_path_handlers.cc:

http://gerrit.cloudera.org:8080/#/c/9253/3/src/kudu/server/default_path_handlers.cc@162
PS3, Line 162: Status s = ListThreads(&tids);
> This just returns Status::OK() on macOS, so the stacks page just says
seems like I can probably change ListThreads to return NotSupported on osx instead, since it's only used here and in some test which doesn't run on osx



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b
Gerrit-Change-Number: 9253
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 13 Feb 2018 21:06:49 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2291 (part 2): Add a /stacks page

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9253 )

Change subject: KUDU-2291 (part 2): Add a /stacks page
......................................................................


Patch Set 3: Code-Review+2

(2 comments)

Nits only, I'd be fine with merging as-is.

http://gerrit.cloudera.org:8080/#/c/9253/3/src/kudu/integration-tests/linked_list-test-util.h
File src/kudu/integration-tests/linked_list-test-util.h:

http://gerrit.cloudera.org:8080/#/c/9253/3/src/kudu/integration-tests/linked_list-test-util.h@315
PS3, Line 315: master_pages.emplace_back("/metrics");
             :     master_pages.emplace_back("/masters");
             :     master_pages.emplace_back("/tables");
             :     master_pages.emplace_back("/dump-entities");
             :     master_pages.emplace_back("/tablet-servers");
             :     master_pages.emplace_back("/mem-trackers");
             :     master_pages.emplace_back("/stacks");
:Puts Adar hat on: Could you sort these in alphabetical order?


http://gerrit.cloudera.org:8080/#/c/9253/3/src/kudu/server/default_path_handlers.cc
File src/kudu/server/default_path_handlers.cc:

http://gerrit.cloudera.org:8080/#/c/9253/3/src/kudu/server/default_path_handlers.cc@162
PS3, Line 162: Status s = ListThreads(&tids);
This just returns Status::OK() on macOS, so the stacks page just says
"Collected stacks from 0 threads in 0.000s".

Not a big deal, but ideally on macOS there'd be a note saying stack collection isn't supported.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b
Gerrit-Change-Number: 9253
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 13 Feb 2018 20:45:33 +0000
Gerrit-HasComments: Yes

[kudu-CR] WIP: KUDU-2291 (part 2): Add a /stacks page

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins, 

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

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

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

Change subject: WIP: KUDU-2291 (part 2): Add a /stacks page
......................................................................

WIP: KUDU-2291 (part 2): Add a /stacks page

This adds a simple /stacks web page which dumps the currently running
threads in a plain text format.

Since we often have a lot of idle threadpool threads sitting around in
the "wait for work" state, the output collapses all threads with
matching stacks and only displays the stack once, making it more
suitable for human consumption.

Example output from a local kudu-master:
  https://gist.github.com/b64739ee5fb146ea1953380f57b996c4

WIP since maybe this should be consolidated with /threadz?

Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b
---
M src/kudu/server/default_path_handlers.cc
1 file changed, 93 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/9253/2
-- 
To view, visit http://gerrit.cloudera.org:8080/9253
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b
Gerrit-Change-Number: 9253
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2291 (part 2): Add a /stacks page

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9253 )

Change subject: KUDU-2291 (part 2): Add a /stacks page
......................................................................

KUDU-2291 (part 2): Add a /stacks page

This adds a simple /stacks web page which dumps the currently running
threads in a plain text format.

Since we often have a lot of idle threadpool threads sitting around in
the "wait for work" state, the output collapses all threads with
matching stacks and only displays the stack once, making it more
suitable for human consumption.

Example output from a local kudu-master:
  https://gist.github.com/b64739ee5fb146ea1953380f57b996c4

Longer term we may want to integrate stack-tracing capability into the
/threadz view as well, but for now I left this as a low-level utility
which doesn't access the thread manager, etc. I left a few TODOs for
further enhancements, but I've already found this helpful for
understanding some perf anomalies while playing with YCSB, so let's get
it committed and improve as we go.

Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b
Reviewed-on: http://gerrit.cloudera.org:8080/9253
Tested-by: Todd Lipcon <to...@apache.org>
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/server/default_path_handlers.cc
M src/kudu/util/debug-util.cc
3 files changed, 106 insertions(+), 9 deletions(-)

Approvals:
  Todd Lipcon: Verified
  Mike Percy: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b
Gerrit-Change-Number: 9253
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>