Surya Hebbar has posted comments on this change. ( http://gerrit.cloudera.org:8080/20355 )
Change subject: IMPALA-12364: Display memory, disk and network metrics in webUI's query timeline ...................................................................... Patch Set 14: (9 comments) http://gerrit.cloudera.org:8080/#/c/20355/13/bin/run-all-tests.sh File bin/run-all-tests.sh: http://gerrit.cloudera.org:8080/#/c/20355/13/bin/run-all-tests.sh@64 PS13, Line 64: # Run JS tests : : ${JS_TEST:=${BE_ > I'm not sure if we should enable this by default now, since this is a new e Done http://gerrit.cloudera.org:8080/#/c/20355/13/www/query_timeline.tmpl File www/query_timeline.tmpl: http://gerrit.cloudera.org:8080/#/c/20355/13/www/query_timeline.tmpl@144 PS13, Line 144: import {renderTimingDiagram, collectFragmentEventsFromProfile} from : "./www/scripts/query_timeline/fragment_diagram.js"; : import {collectUtilizationFromProfile, toogleUtilizationVisibility} from : "/www/scripts/query_timeline/host_utilization_diagram.js"; : import {collectFragmentMetricsFromProfile} from : "/www/scripts/query_timeline/fragment_metrics_diagram.js"; : import {profile, set_profile, diagram_width, set_diagram_width, border_stroke_width, : resizeHorizontalAll, maxts} from "/www/scripts/query_timeline/global_members.js"; > nit: newbie JS question: why these JS scripts imported this way instead of As we are using 'import' and 'export' statements in the other JS files, which are part of ES6 modules and ECMA standard, we cannot use the usual <script> tag or commonJS imports. It produces errors. The reason for using ES6 modules was to write unit tests with isolation and without polluting the namespace. Without imports and exports, as there are too many references to DOM and other dependencies, writing tests without ES6 is equivalent to writing repetitive test code or integration tests. http://gerrit.cloudera.org:8080/#/c/20355/13/www/scripts/query_timeline/chart_commons.test.js File www/scripts/query_timeline/chart_commons.test.js: http://gerrit.cloudera.org:8080/#/c/20355/13/www/scripts/query_timeline/chart_commons.test.js@309 PS13, Line 309: host-1:27000", > nit: Is this a hostname? If so, maybe consider changing it to something lik Done http://gerrit.cloudera.org:8080/#/c/20355/13/www/scripts/query_timeline/fragment_diagram.js File www/scripts/query_timeline/fragment_diagram.js: http://gerrit.cloudera.org:8080/#/c/20355/13/www/scripts/query_timeline/fragment_diagram.js@92 PS13, Line 92: height / 1.5 > nit: syntax color look messed up in gerrit starting from here. Is this OK? For me, I am not experiencing syntax highlighting problems here or do not realize the problem. I'm on the old UI and the version is 2.14.15. I can contain it as a variable. http://gerrit.cloudera.org:8080/#/c/20355/13/www/scripts/query_timeline/fragment_diagram.js@332 PS13, Line 332: // First pass: compute si > nit: console.assert the name of profile.child_profiles[2]? Maybe address it Done http://gerrit.cloudera.org:8080/#/c/20355/13/www/scripts/query_timeline/fragment_diagram.js@499 PS13, Line 499: h = max_namelen * char_width + (frag_name_width + 2); : fragment_events_parse_successful = > nit: can this be a variable? Done http://gerrit.cloudera.org:8080/#/c/20355/13/www/scripts/query_timeline/fragment_metrics_diagram.js File www/scripts/query_timeline/fragment_metrics_diagram.js: http://gerrit.cloudera.org:8080/#/c/20355/13/www/scripts/query_timeline/fragment_metrics_diagram.js@112 PS13, Line 112: name_width + chart_width / 4 > nit: syntax color look messed up in gerrit starting from here. Is this OK? For me, I am not experiencing syntax highlighting problems here or do not realize the problem. I'm on the old UI and the version is 2.14.15. I can contain it as a variable. http://gerrit.cloudera.org:8080/#/c/20355/13/www/scripts/query_timeline/host_utilization_diagram.js File www/scripts/query_timeline/host_utilization_diagram.js: http://gerrit.cloudera.org:8080/#/c/20355/13/www/scripts/query_timeline/host_utilization_diagram.js@222 PS13, Line 222: and do not render th > nit: Add console.assert("Per Node Profiles", per_node_profiles); Done http://gerrit.cloudera.org:8080/#/c/20355/13/www/scripts/query_timeline/host_utilization_diagram.js@231 PS13, Line 231: per_node_profiles, cpu_ > nit: Address profile.child_profiles[1] as a descriptive variable and consol Done -- To view, visit http://gerrit.cloudera.org:8080/20355 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifd25e6f0bc9fbd664ec98936daff3f27182dfc7f Gerrit-Change-Number: 20355 Gerrit-PatchSet: 14 Gerrit-Owner: Surya Hebbar <sheb...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Surya Hebbar <sheb...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Mon, 25 Sep 2023 18:51:37 +0000 Gerrit-HasComments: Yes
