You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "KeDeng (Code Review)" <ge...@cloudera.org> on 2022/12/28 07:34:47 UTC

[kudu-CR] [www] add slow scans show

KeDeng has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19392


Change subject: [www] add slow scans show
......................................................................

[www] add slow scans show

We can get historical completed scans from /scans page right now.
But it is not convenient for us to get slow scans.

I change the previous scan presentation into completed scan show and
add slow scans show. Slow scan defined only by the time used, which
threshold is --slow_scanner_threshold_ms.
The number of slow scans show is limit by --slow_scan_history_count.

Change-Id: Ibd1dcac8b81c5eefd306e7020c9c52b3f28e603c
---
M src/kudu/client/scan_token-test.cc
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tserver_path_handlers.cc
M www/scans.mustache
6 files changed, 292 insertions(+), 90 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibd1dcac8b81c5eefd306e7020c9c52b3f28e603c
Gerrit-Change-Number: 19392
Gerrit-PatchSet: 1
Gerrit-Owner: KeDeng <kd...@gmail.com>

[kudu-CR] [www] add slow scans section

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19392 )

Change subject: [www] add slow scans section
......................................................................

Patch Set 14: Code-Review+2

(4 comments)

http://gerrit.cloudera.org:8080/#/c/19392/11//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19392/11//COMMIT_MSG@17
PS11, Line 17:
> Yes, this is another implementation. I have also considered it. After all,
Ah, I see -- thanks for the clarification. Indeed, then it makes sense to implement this exactly the way you did.

http://gerrit.cloudera.org:8080/#/c/19392/11/src/kudu/tserver/scanners.h
File src/kudu/tserver/scanners.h:

http://gerrit.cloudera.org:8080/#/c/19392/11/src/kudu/tserver/scanners.h@479
PS11, Line 479:
> The 'RefCounted' is my choice in the alpha version. In this version, there
Yup, that makes sense to me. Thank you for the clarification!

http://gerrit.cloudera.org:8080/#/c/19392/11/src/kudu/tserver/scanners.cc
File src/kudu/tserver/scanners.cc:

http://gerrit.cloudera.org:8080/#/c/19392/11/src/kudu/tserver/scanners.cc@361
PS11, Line 361: "Slow scanner id: $0, of tablet $1, "
: "exceed the time threshold $2 ms for $3 ms.",
: it->first,
: scanner->tablet_id(),
: slow_scanner_threshold,
: delta_time.ToMilliseconds());
: descriptors.emplace_back(scanner-
> What I hope is that there are ways to obtain these slow scans logs if neces
If the idea was to track those scanner and found them a long time after there were running, I guess that's the right way of doing that. I didn't realize that was the intention given that this patch already updates the 'Scans' page of the embedded webserver.

With that, this looks good enough to me. I guess if people found their logs to be polluted with such messages, they can customize the corresponding flag.

VLOG(1) or VLOG(2) is also an option, but you'd need to set verbose mode beforehand.

http://gerrit.cloudera.org:8080/#/c/19392/14/src/kudu/tserver/scanners.cc
File src/kudu/tserver/scanners.cc:

http://gerrit.cloudera.org:8080/#/c/19392/14/src/kudu/tserver/scanners.cc@360
PS14, Line 360: VLOG(2)
With the explanation you provided at https://gerrit.cloudera.org/#/c/19392/11/src/kudu/tserver/scanners.cc@367 , it seems VLOG(1) or even LOG(INFO) might be a better option than VLOG(2). VLOG(2) contains too much stuff from elsewhere.

Maybe, this should be configurable whether to log about slow scans or not? Say, if that's gated by a run-time flag, then there is no risk of polluting the log even if the default slowness threshold isn't adjusted for a particular workload pattern when logging with LOG(INFO) here.

Let me know if you want to address this here or in a separate patch. Either option looks good to me. Thanks!

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd1dcac8b81c5eefd306e7020c9c52b3f28e603c
Gerrit-Change-Number: 19392
Gerrit-PatchSet: 14
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 07 Feb 2023 04:53:51 +0000
Gerrit-HasComments: Yes