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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 07 Feb 2023 04:53:51 +0000 Gerrit-HasComments: Yes
