Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21593 )
Change subject: IMPALA-13233: Improve display of instance-level skew in query timeline ...................................................................... Patch Set 8: (12 comments) http://gerrit.cloudera.org:8080/#/c/21593/5/www/scripts/query_timeline/fragment_diagram.js File www/scripts/query_timeline/fragment_diagram.js: http://gerrit.cloudera.org:8080/#/c/21593/5/www/scripts/query_timeline/fragment_diagram.js@86 PS5, Line 86: rect.setAttribute("x", rtd(x)); : rect.setAttribute("y", rtd(y)); : rect.setAttribute("width", rtd(width)); : rect.setAttribute("height", rtd(height)); : rect.setAttribute("fill", fill_color); : r > I have treied setting this attribute to 'none', this would unnecessarily in Done http://gerrit.cloudera.org:8080/#/c/21593/5/www/scripts/query_timeline/fragment_diagram.js@95 PS5, Line 95: > I have passed '0' now to make things consistent. Done http://gerrit.cloudera.org:8080/#/c/21593/5/www/scripts/query_timeline/fragment_diagram.js@194 PS5, Line 194: var bar_height = row_height - 2; : var last_e_index = events.length - 1; : var last_ts > I am very sorry for the confusion caused. Done http://gerrit.cloudera.org:8080/#/c/21593/5/www/scripts/query_timeline/fragment_diagram.js@199 PS5, Line 199: > Now, I have replaced it with 'bucket_size'. As the bucket size is not a max Ack. Please file a follow up JIRA for code style guide and refactoring. http://gerrit.cloudera.org:8080/#/c/21593/5/www/scripts/query_timeline/fragment_diagram.js@225 PS5, Line 225: bucketed) { > I missed mentioning the proportional height relation in the commit message. Done http://gerrit.cloudera.org:8080/#/c/21593/5/www/scripts/query_timeline/fragment_diagram.js@227 PS5, Line 227: r part; : for (j = 0; j < events[i].parts.length; j++) { : part = events[i].parts[j]; // mantain a reference for reuse : if (part.count > 0) { > I broke down the long confusing string for 'stroke-dasharray' into a variab Done http://gerrit.cloudera.org:8080/#/c/21593/8/www/scripts/query_timeline/fragment_diagram.js File www/scripts/query_timeline/fragment_diagram.js: http://gerrit.cloudera.org:8080/#/c/21593/8/www/scripts/query_timeline/fragment_diagram.js@84 PS8, Line 84: function getSvgRect(fill_color, x, y, width, height, dasharray, stroke_color) { nit: Can you specify default value in JS? function getSvgRect(fill_color, x, y, width, height, dasharray=undefined, stroke_color=undefined) { http://gerrit.cloudera.org:8080/#/c/21593/8/www/scripts/query_timeline/fragment_diagram.js@112 PS8, Line 112: max_width nit: max_width > 0 http://gerrit.cloudera.org:8080/#/c/21593/8/www/scripts/query_timeline/fragment_diagram.js@199 PS8, Line 199: var y = rownum * row_height; // the y-position for the current phase rectangle (0,0) coordinate is top-left or bottom-left? Please document that as a comment before this line. http://gerrit.cloudera.org:8080/#/c/21593/8/www/scripts/query_timeline/fragment_diagram.js@233 PS8, Line 233: // skip to the next iteration, if the current bucket has no timestamps Move this comment inside parenthesis of L232. http://gerrit.cloudera.org:8080/#/c/21593/8/www/scripts/query_timeline/fragment_diagram.js@239 PS8, Line 239: `0 ${rtd(l_dx)} ${rtd(dx - l_dx + dy)} ${rtd(dx)} ${rtd(dy)} 0` Consider wrapping this into a function to ensure same formula at L239 and L252. http://gerrit.cloudera.org:8080/#/c/21593/8/www/scripts/query_timeline/fragment_diagram.js@435 PS8, Line 435: undefined nit: 0 -- To view, visit http://gerrit.cloudera.org:8080/21593 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ied8a5966e9e4111bf7aa25aee11d23881daad7d2 Gerrit-Change-Number: 21593 Gerrit-PatchSet: 8 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-Comment-Date: Tue, 17 Sep 2024 17:57:38 +0000 Gerrit-HasComments: Yes
