Surya Hebbar has posted comments on this change. ( http://gerrit.cloudera.org:8080/22245 )
Change subject: IMPALA-13624: Implement textual representation for aggregate event sequences ...................................................................... Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/22245/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22245/5//COMMIT_MSG@19 PS5, Line 19: : The su > Spell it out in commit message: "in histogram format". Done. I have updated the message now. http://gerrit.cloudera.org:8080/#/c/22245/5//COMMIT_MSG@37 PS5, Line 37: : Node Lifecycle Event Timeline: : - Open Started: 5s885ms, 1s708ms, 3s434ms > The histogram can be more self explanatory if Interval and Width is also pr The formats present in PS 3 to 5 were to reduce the number of lines in the profile, as suggested during a sync. I have implemented it in this format now. Thank you for the suggestion. http://gerrit.cloudera.org:8080/#/c/22245/5/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/22245/5/be/src/util/runtime-profile.cc@2755 PS5, Line 2755: { > Unnecessary scope? This is to make sure the copied agg_event_sequences and any other related temporary structures are out of scope once done. http://gerrit.cloudera.org:8080/#/c/22245/5/be/src/util/runtime-profile.cc@2785 PS5, Line 2785: for (const auto& seq : agg_event_sequences) { > Why is it need to hold lock again here if you have copy event_sequence_map_ Sorry, had forgotten to remove it, the entire reason for transformation was to avoid this. http://gerrit.cloudera.org:8080/#/c/22245/5/be/src/util/runtime-profile.cc@2818 PS5, Line 2818: if (event_sequence_json.HasMember("events") && > I'm concern about the performance here because it converts into rapidjson f 'AggEventSequence' is the common struct/data structure that holds values efficiently. For generating the summary, this can either directly be converted to text or to a volatile data structure in between such as JSON. As this summarized structure may change, and to also cover the possibility of missing events and order, using the volatile JSON format is possibly the best choice. http://gerrit.cloudera.org:8080/#/c/22245/5/be/src/util/runtime-profile.cc@2832 PS5, Line 2832: ostringstream interval_summary_l > Reuse this stringstream by moving the declaration before L2816 loop. Done -- To view, visit http://gerrit.cloudera.org:8080/22245 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4bcc0e2e7fccfa8a184cfa8a3a96d68bfe6035c0 Gerrit-Change-Number: 22245 Gerrit-PatchSet: 6 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: Mon, 07 Apr 2025 09:57:05 +0000 Gerrit-HasComments: Yes
