Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20008 )
Change subject: IMPALA-12182: Add CPU utilization chart for RuntimeProfile's sampled metrics ...................................................................... Patch Set 2: (12 comments) http://gerrit.cloudera.org:8080/#/c/20008/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20008/1//COMMIT_MSG@9 PS1, Line 9: diagram : > Okay. I will mention the query option and associated counter names. Done http://gerrit.cloudera.org:8080/#/c/20008/1//COMMIT_MSG@15 PS1, Line 15: > Sorry, I was referring to the thrift profile mentioned on the query profile Done http://gerrit.cloudera.org:8080/#/c/20008/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20008/2//COMMIT_MSG@6 PS2, Line 6: : IMPALA-12182: Add CPU utilization chart for RuntimeProfile's sampled metrics : : IMPALA-12230: Support scaling of timeticks and fragment timing diagram nit: Meld commit title into one? IMPALA-12182,IMPALA-12230: .... http://gerrit.cloudera.org:8080/#/c/20008/2//COMMIT_MSG@41 PS2, Line 41: These are retrieved from the ChunkedTimeSeriesCounter in the : RuntimeProfile. nit: Is it correct that this path is not working yet with v2 profile (-gen_experimental_profile=true)? Can you make it work with V2 profile? If not, please mention it in the commit message. http://gerrit.cloudera.org:8080/#/c/20008/1/www/query_timeline.tmpl File www/query_timeline.tmpl: http://gerrit.cloudera.org:8080/#/c/20008/1/www/query_timeline.tmpl@559 PS1, Line 559: t > Okay. Done http://gerrit.cloudera.org:8080/#/c/20008/1/www/query_timeline.tmpl@563 PS1, Line 563: > Sure. This was being taken from AggregatedRuntimeProfile's number of sample Done http://gerrit.cloudera.org:8080/#/c/20008/1/www/query_timeline.tmpl@581 PS1, Line 581: > Okay. Done http://gerrit.cloudera.org:8080/#/c/20008/1/www/query_timeline.tmpl@688 PS1, Line 688: nt_diagram_wrapper.style.heig > Thank you.This would be useful. Currently, if the RESOURCE_TRACE_RATIO was Done http://gerrit.cloudera.org:8080/#/c/20008/2/www/query_timeline.tmpl File www/query_timeline.tmpl: http://gerrit.cloudera.org:8080/#/c/20008/2/www/query_timeline.tmpl@408 PS2, Line 408: diagram_width + "px" nit: multiple concats of + "px" around. Seems worth to make it into its own function, like to_px(diagram_width) http://gerrit.cloudera.org:8080/#/c/20008/2/www/query_timeline.tmpl@430 PS2, Line 430: #000000 nit: How about make string constants for all RGB values like this? Maybe include the target component or color name into the constant name. http://gerrit.cloudera.org:8080/#/c/20008/2/www/query_timeline.tmpl@578 PS2, Line 578: nit: unnecessary whitespace. http://gerrit.cloudera.org:8080/#/c/20008/2/www/query_timeline.tmpl@713 PS2, Line 713: if ( e.shiftKey ) { : if ( e.target.parentElement.id === "timeticks_footer" ) { : if ( e.altKey ) { : if (decimals <= 1 && e.wheelDelta <= 0) return; : if (decimals >= 6 && e.wheelDelta >= 0) return; : if (e.wheelDelta >= 0) { nit: here and other places, still inconsistent whitespace after "if (". Don't think we have strict codestyle for JS code, but best to stick without whitespace. -- To view, visit http://gerrit.cloudera.org:8080/20008 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idea2a6db217dbfaa7a0695aeabb6d9c1ecf62158 Gerrit-Change-Number: 20008 Gerrit-PatchSet: 2 Gerrit-Owner: Surya Hebbar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Tue, 20 Jun 2023 23:44:51 +0000 Gerrit-HasComments: Yes
