Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21291 )
Change subject: [metrics] Add tablet level metrics for scans op time ...................................................................... Patch Set 5: Code-Review+1 (12 comments) Almost there, just a style and wording nits. Thank you for revving the patch! http://gerrit.cloudera.org:8080/#/c/21291/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21291/5//COMMIT_MSG@10 PS5, Line 10: scan time nit: scan request timings That's less ambiguous since 'scan time' might be confused with the total time spent to fetch all the data from all the involved tablet servers based on the scan's spec. >From what I could see, this patch adds histogram metrics to track timings of >individual scan requests (both new and continuation ones). http://gerrit.cloudera.org:8080/#/c/21291/5//COMMIT_MSG@11 PS5, Line 11: scan time nit: scan request timings http://gerrit.cloudera.org:8080/#/c/21291/5/src/kudu/tablet/tablet_metrics.h File src/kudu/tablet/tablet_metrics.h: http://gerrit.cloudera.org:8080/#/c/21291/5/src/kudu/tablet/tablet_metrics.h@68 PS5, Line 68: scoped_refptr<Histogram> tablet_scans_duration_wall_time; : scoped_refptr<Histogram> tablet_scans_duration_system_time; : scoped_refptr<Histogram> tablet_scans_duration_user_time; These metrics are already in the tablet context, similar to all the other fields in this TabletMetrics struct. With that, maybe rename these to match the others: scan_duration_system_time scan_duration_user_time scan_duration_wall_time http://gerrit.cloudera.org:8080/#/c/21291/5/src/kudu/tablet/tablet_metrics.cc File src/kudu/tablet/tablet_metrics.cc: http://gerrit.cloudera.org:8080/#/c/21291/5/src/kudu/tablet/tablet_metrics.cc@157 PS5, Line 157: Tablet Scans Duration In Wall Time Scan Requests Wall Time http://gerrit.cloudera.org:8080/#/c/21291/5/src/kudu/tablet/tablet_metrics.cc@159 PS5, Line 159: Wall time spent scanners scans Duration of scan requests, wall time http://gerrit.cloudera.org:8080/#/c/21291/5/src/kudu/tablet/tablet_metrics.cc@164 PS5, Line 164: Tablet Scans Duration In System Time Scan Requests System Time http://gerrit.cloudera.org:8080/#/c/21291/5/src/kudu/tablet/tablet_metrics.cc@166 PS5, Line 166: System time spent scanners scans. Duration of scan requests, system time http://gerrit.cloudera.org:8080/#/c/21291/5/src/kudu/tablet/tablet_metrics.cc@171 PS5, Line 171: Tablet Scans Duration In User Time Scan Requests User Time http://gerrit.cloudera.org:8080/#/c/21291/5/src/kudu/tablet/tablet_metrics.cc@173 PS5, Line 173: User time spent scanners scans Duration of scan requests, user time http://gerrit.cloudera.org:8080/#/c/21291/5/src/kudu/tserver/scanners.h File src/kudu/tserver/scanners.h: http://gerrit.cloudera.org:8080/#/c/21291/5/src/kudu/tserver/scanners.h@410 PS5, Line 410: Add the timings in 'elapsed' to the metrics of tablet. Store the scan request's timings in the tablet metrics. http://gerrit.cloudera.org:8080/#/c/21291/5/src/kudu/tserver/scanners.h@411 PS5, Line 411: update_tablet_metrics style nit: since this isn't an accessor/getter, this should be named like UpdateTabletMetrics() http://gerrit.cloudera.org:8080/#/c/21291/5/src/kudu/tserver/scanners.cc File src/kudu/tserver/scanners.cc: http://gerrit.cloudera.org:8080/#/c/21291/5/src/kudu/tserver/scanners.cc@529 PS5, Line 529: The time spent on this scanner scan nit: Store scan request's timings -- To view, visit http://gerrit.cloudera.org:8080/21291 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f490cfb6f37aef60b34697100fb502374fcc503 Gerrit-Change-Number: 21291 Gerrit-PatchSet: 5 Gerrit-Owner: KeDeng <kdeng...@gmail.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: KeDeng <kdeng...@gmail.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org> Gerrit-Comment-Date: Thu, 23 May 2024 03:49:47 +0000 Gerrit-HasComments: Yes
