Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21851 )
Change subject: IMPALA-13389: Refactor webUI scripts to use ES6 syntax ...................................................................... Patch Set 14: (4 comments) Please search for unused/commented code and consider removing them. http://gerrit.cloudera.org:8080/#/c/21851/14/www/scripts/query_timeline/fragment_diagram.js File www/scripts/query_timeline/fragment_diagram.js: http://gerrit.cloudera.org:8080/#/c/21851/14/www/scripts/query_timeline/fragment_diagram.js@201 PS14, Line 201: // const LAST_TS_INDEX = events[last_e_index].ts_list.length - 1; Can this be removed? http://gerrit.cloudera.org:8080/#/c/21851/14/www/scripts/query_timeline/fragment_diagram.js@271 PS14, Line 271: /* : y = rownum * row_height; : if (events[last_e_index].no_bar === undefined) { : // Plan node's top and bottom outlines : let top_edge_ts, bottom_edge_ts; : if (bucketed) { : top_edge_ts = events[last_e_index].parts[0].max; : bottom_edge_ts = events[last_e_index].parts[bucket_size - 1].max; : } else { : top_edge_ts = events[last_e_index].ts_list[0]; : bottom_edge_ts = events[last_e_index].ts_list[last_ts_index]; : } : plan_node.appendChild(getSvgLine(stroke_fill_colors.black, xoffset, y, : xoffset + top_edge_ts * px_per_ns, y, false)); : plan_node.appendChild(getSvgLine(stroke_fill_colors.black, xoffset, y + bar_height, : xoffset + bottom_edge_ts * px_per_ns, y + bar_height, : false)); : }*/ Can this be removed? http://gerrit.cloudera.org:8080/#/c/21851/14/www/scripts/query_timeline/fragment_diagram.js@609 PS14, Line 609: //node = node_stack.pop(); Can this be removed? http://gerrit.cloudera.org:8080/#/c/21851/14/www/scripts/query_timeline/global_members.js File www/scripts/query_timeline/global_members.js: http://gerrit.cloudera.org:8080/#/c/21851/14/www/scripts/query_timeline/global_members.js@22 PS14, Line 22: export const BORDER_STROKE_WIDTH = 2; : export const MARGIN_HEADER_FOOTER = 5; : export const MARGIN_CHART_END = 60; : export const DIAGRAM_MIN_HEIGHT = 200; Is it appropriate to prefix all this constants with QUERY_TIMELINE_? It looks like they are exclusively used in timeline page, so that prefix can increase clarity. Otherwise, please add code comment explaining where/how they are being used. -- To view, visit http://gerrit.cloudera.org:8080/21851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie38f2c642ede14956a2c6d551a58e42538204768 Gerrit-Change-Number: 21851 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: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Surya Hebbar <sheb...@cloudera.com> Gerrit-Comment-Date: Thu, 20 Mar 2025 14:08:52 +0000 Gerrit-HasComments: Yes
