You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2019/10/16 23:47:00 UTC
[kudu-CR] www: convert dashboards and threads to mustache templates
Hello Alexey Serbin, Andrew Wong,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/14473
to review the following change.
Change subject: www: convert dashboards and threads to mustache templates
......................................................................
www: convert dashboards and threads to mustache templates
While I was there I added "fancy tables" to /threadz.
Change-Id: Idb97a9e3bbefb8ee607638af6e069959c5354225
---
M src/kudu/tserver/tserver_path_handlers.cc
M src/kudu/tserver/tserver_path_handlers.h
M src/kudu/util/thread.cc
A www/dashboards.mustache
M www/kudu.js
M www/tablets.mustache
A www/threadz.mustache
7 files changed, 165 insertions(+), 106 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/14473/1
--
To view, visit http://gerrit.cloudera.org:8080/14473
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Idb97a9e3bbefb8ee607638af6e069959c5354225
Gerrit-Change-Number: 14473
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
[kudu-CR] www: miscellaneous mustache updates
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14473
to look at the new patch set (#3).
Change subject: www: miscellaneous mustache updates
......................................................................
www: miscellaneous mustache updates
1. Converted /dashboards and /threadz into mustache templates. In /threadz,
I tried to defer as much work as possible outside the lock, since it must
be taken in write mode to create a new thread. While I was there I added
"fancy tables" to /threadz; /dashboards remains visually unchanged.
2. In /tablets, changed the handling of tablet links. In doing so, all links
(except for the main template) are now in templates instead of in code.
Screenshots: https://imgur.com/a/AOTQuH4
Change-Id: Idb97a9e3bbefb8ee607638af6e069959c5354225
---
M src/kudu/tserver/tserver_path_handlers.cc
M src/kudu/tserver/tserver_path_handlers.h
M src/kudu/util/thread.cc
A www/dashboards.mustache
M www/kudu.js
M www/tablets.mustache
A www/threadz.mustache
7 files changed, 192 insertions(+), 105 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/14473/3
--
To view, visit http://gerrit.cloudera.org:8080/14473
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb97a9e3bbefb8ee607638af6e069959c5354225
Gerrit-Change-Number: 14473
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] www: miscellaneous mustache updates
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14473 )
Change subject: www: miscellaneous mustache updates
......................................................................
www: miscellaneous mustache updates
1. Converted /dashboards and /threadz into mustache templates. In /threadz,
I tried to defer as much work as possible outside the lock, since it must
be taken in write mode to create a new thread. While I was there I added
"fancy tables" to /threadz; /dashboards remains visually unchanged.
2. In /tablets, changed the handling of tablet links. In doing so, all links
(except for the main template) are now in templates instead of in code.
Screenshots: https://imgur.com/a/AOTQuH4
Change-Id: Idb97a9e3bbefb8ee607638af6e069959c5354225
Reviewed-on: http://gerrit.cloudera.org:8080/14473
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/tserver/tserver_path_handlers.cc
M src/kudu/tserver/tserver_path_handlers.h
M src/kudu/util/thread.cc
A www/dashboards.mustache
M www/kudu.js
M www/tablets.mustache
A www/threadz.mustache
7 files changed, 192 insertions(+), 107 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14473
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Idb97a9e3bbefb8ee607638af6e069959c5354225
Gerrit-Change-Number: 14473
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] www: miscellaneous mustache updates
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14473 )
Change subject: www: miscellaneous mustache updates
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14473/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14473/1//COMMIT_MSG@9
PS1, Line 9: 1. Converted /dashboards and /threadz into mustache t
> Could you post a snippet of now it looks like now?
Added link to screenshots.
/tablets was already mustached, but I'll mention the change I made to it.
http://gerrit.cloudera.org:8080/#/c/14473/1/src/kudu/util/thread.cc
File src/kudu/util/thread.cc:
http://gerrit.cloudera.org:8080/#/c/14473/1/src/kudu/util/thread.cc@388
PS1, Line 388: thr["user_sec"] = static_cast<double>(stats.us
> Is this needed?
That's a fair criticism, and I think the issue isn't so much in EasyJson or ostringstream as it is with the behavior of PrintThreadDescriptorRow, which does several syscalls on procfs.
I'll fix this.
http://gerrit.cloudera.org:8080/#/c/14473/1/www/threadz.mustache
File www/threadz.mustache:
http://gerrit.cloudera.org:8080/#/c/14473/1/www/threadz.mustache@28
PS1, Line 28: (s
> nit here and below: '(s)' stands for seconds here, right? Maybe, separate
Separated with a space.
When repeated three times, '(seconds)' makes the table quite wide, so I'm punting on that.
--
To view, visit http://gerrit.cloudera.org:8080/14473
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb97a9e3bbefb8ee607638af6e069959c5354225
Gerrit-Change-Number: 14473
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 17 Oct 2019 06:43:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] www: miscellaneous mustache updates
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14473 )
Change subject: www: miscellaneous mustache updates
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14473
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb97a9e3bbefb8ee607638af6e069959c5354225
Gerrit-Change-Number: 14473
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 18 Oct 2019 00:05:55 +0000
Gerrit-HasComments: No
[kudu-CR] www: miscellaneous mustache updates
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14473
to look at the new patch set (#2).
Change subject: www: miscellaneous mustache updates
......................................................................
www: miscellaneous mustache updates
1. Converted /dashboards and /threadz into mustache templates. In /threadz,
I tried to defer as much work as possible outside the lock, since it must
be taken in write mode to create a new thread. While I was there I added
"fancy tables" to /threadz; /dashboards remains visually unchanged.
2. In /tablets, changed the handling of tablet links. In doing so, all links
(except for the main template) are now in templates instead of in code.
Screenshots: https://imgur.com/a/AOTQuH4
Change-Id: Idb97a9e3bbefb8ee607638af6e069959c5354225
---
M src/kudu/tserver/tserver_path_handlers.cc
M src/kudu/tserver/tserver_path_handlers.h
M src/kudu/util/thread.cc
A www/dashboards.mustache
M www/kudu.js
M www/tablets.mustache
A www/threadz.mustache
7 files changed, 185 insertions(+), 97 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/14473/2
--
To view, visit http://gerrit.cloudera.org:8080/14473
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb97a9e3bbefb8ee607638af6e069959c5354225
Gerrit-Change-Number: 14473
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] www: miscellaneous mustache updates
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14473 )
Change subject: www: miscellaneous mustache updates
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14473/2/src/kudu/tserver/tserver_path_handlers.cc
File src/kudu/tserver/tserver_path_handlers.cc:
http://gerrit.cloudera.org:8080/#/c/14473/2/src/kudu/tserver/tserver_path_handlers.cc@345
PS2, Line 345: "url"
nit: maybe swap this with "link"? I generally think of "links" a the clickable object and the "url" as what goes into the url bar.
http://gerrit.cloudera.org:8080/#/c/14473/2/src/kudu/util/thread.cc
File src/kudu/util/thread.cc:
http://gerrit.cloudera.org:8080/#/c/14473/2/src/kudu/util/thread.cc@309
PS2, Line 309: d*
nit: odd spacing here
http://gerrit.cloudera.org:8080/#/c/14473/2/src/kudu/util/thread.cc@378
PS2, Line 378: PrintThreadDescriptorRow
nit: maybe rename this to SummarizeThreadDescriptor or somesuch?
http://gerrit.cloudera.org:8080/#/c/14473/2/src/kudu/util/thread.cc@410
PS2, Line 410: const auto it = thread_categories_.find(group);
: if (it == thread_categories_.end()) {
: return;
: }
nit: would ContainsKey work here?
--
To view, visit http://gerrit.cloudera.org:8080/14473
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb97a9e3bbefb8ee607638af6e069959c5354225
Gerrit-Change-Number: 14473
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 17 Oct 2019 07:55:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] www: convert dashboards and threads to mustache templates
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14473 )
Change subject: www: convert dashboards and threads to mustache templates
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14473/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14473/1//COMMIT_MSG@9
PS1, Line 9: While I was there I added "fancy tables" to /threadz.
Could you post a snippet of now it looks like now?
Also, what about /tablets?
http://gerrit.cloudera.org:8080/#/c/14473/1/src/kudu/util/thread.cc
File src/kudu/util/thread.cc:
http://gerrit.cloudera.org:8080/#/c/14473/1/src/kudu/util/thread.cc@388
PS1, Line 388: vector<ThreadDescriptor> descriptors_to_print;
Is this needed?
BTW, in prior approach lock was only held to make a snapshot of the thread descriptors, etc. The idea was to avoid expensive IO (writing into ostringstream) while holding the lock, because it might hamper adding information on spawned threads into the thread registry. How is expensive to work with EasyJson in that regard?
http://gerrit.cloudera.org:8080/#/c/14473/1/www/threadz.mustache
File www/threadz.mustache:
http://gerrit.cloudera.org:8080/#/c/14473/1/www/threadz.mustache@28
PS1, Line 28: (s)
nit here and below: '(s)' stands for seconds here, right? Maybe, separate '(s)' with space from the prefix. Additionally, maybe replace '(s)' with '(seconds)'?
--
To view, visit http://gerrit.cloudera.org:8080/14473
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb97a9e3bbefb8ee607638af6e069959c5354225
Gerrit-Change-Number: 14473
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 17 Oct 2019 00:32:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] www: miscellaneous mustache updates
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14473
to look at the new patch set (#4).
Change subject: www: miscellaneous mustache updates
......................................................................
www: miscellaneous mustache updates
1. Converted /dashboards and /threadz into mustache templates. In /threadz,
I tried to defer as much work as possible outside the lock, since it must
be taken in write mode to create a new thread. While I was there I added
"fancy tables" to /threadz; /dashboards remains visually unchanged.
2. In /tablets, changed the handling of tablet links. In doing so, all links
(except for the main template) are now in templates instead of in code.
Screenshots: https://imgur.com/a/AOTQuH4
Change-Id: Idb97a9e3bbefb8ee607638af6e069959c5354225
---
M src/kudu/tserver/tserver_path_handlers.cc
M src/kudu/tserver/tserver_path_handlers.h
M src/kudu/util/thread.cc
A www/dashboards.mustache
M www/kudu.js
M www/tablets.mustache
A www/threadz.mustache
7 files changed, 192 insertions(+), 107 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/14473/4
--
To view, visit http://gerrit.cloudera.org:8080/14473
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb97a9e3bbefb8ee607638af6e069959c5354225
Gerrit-Change-Number: 14473
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] www: miscellaneous mustache updates
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14473 )
Change subject: www: miscellaneous mustache updates
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14473
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb97a9e3bbefb8ee607638af6e069959c5354225
Gerrit-Change-Number: 14473
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 17 Oct 2019 21:38:16 +0000
Gerrit-HasComments: No