Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21683 )
Change subject: IMPALA-13304: Include aggregate instance-level metrics within experimental profile(V2) ...................................................................... Patch Set 2: (10 comments) http://gerrit.cloudera.org:8080/#/c/21683/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21683/2//COMMIT_MSG@21 PS2, Line 21: than 4 Is it suppose to be 5 or 4? http://gerrit.cloudera.org:8080/#/c/21683/2//COMMIT_MSG@37 PS2, Line 37: When no. of instances > 4 - I rather be consistent with displaying this in group-of-4 formats for any case. If there are less than 4 instances, the min, max, avg, and count list length should be less than 3, but equal length. http://gerrit.cloudera.org:8080/#/c/21683/2//COMMIT_MSG@80 PS2, Line 80: : Note: In the above structures, unlike a plan node's profile, : a fragment's profile does not contain the 'node_metadata' field. Please run tests/observability/test_profile_tool.py with this patch. The test producing json profile should change, I think. http://gerrit.cloudera.org:8080/#/c/21683/2/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: http://gerrit.cloudera.org:8080/#/c/21683/2/be/src/util/runtime-profile.h@940 PS2, Line 940: typedef std::map<std::string, AggEventSequence> EventSequenceMap; : EventSequenceMap event_sequence_map_; Unnecessary change? EventSequenceMap is not referred anywhere else. http://gerrit.cloudera.org:8080/#/c/21683/2/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/21683/2/be/src/util/runtime-profile.cc@a2925 PS2, Line 2925: : : This does not need to change, right? http://gerrit.cloudera.org:8080/#/c/21683/2/be/src/util/runtime-profile.cc@2927 PS2, Line 2927: // If number of instances is more than 4, aggregate of event timestamps (i.e. Average, : // Minimum and Maximum) is included after bucketing the instances into 4 groups. : // In other cases, timestamp of all 4 or less instances is included. Put "Event sequence" title, like SummaryStatsCounter comment before this. http://gerrit.cloudera.org:8080/#/c/21683/2/be/src/util/runtime-profile.cc@2957 PS2, Line 2957: e_s.second Create a descriptive variable name to refer e_s.second and use it everywhere else. http://gerrit.cloudera.org:8080/#/c/21683/2/be/src/util/runtime-profile.cc@2963 PS2, Line 2963: int8_t int should be fine here and others below. http://gerrit.cloudera.org:8080/#/c/21683/2/be/src/util/runtime-profile.cc@2968 PS2, Line 2968: int8_t events_count = e_s.second.labels.size(); Move this before L2957 and refer to events_count variable anywhere else. http://gerrit.cloudera.org:8080/#/c/21683/2/be/src/util/runtime-profile.cc@2979 PS2, Line 2979: size_t prev_j = 0; : for (int8_t k = 0; k < 4; k++) { : min = max = avg = e_s.second.timestamps[prev_j][i]; : for (size_t j = prev_j + 1; j < prev_j + div_insts[k]; j++) { Some of the iterator variable here can be made clearer with more descriptive name. Here is what I am suggesting, and please correct me if I'm wrong: i : event_idx j : instance_idx k : grouping_idx prest_j : last_event_idx div_insts : instance_count_in_group no_insts : num_instances -- To view, visit http://gerrit.cloudera.org:8080/21683 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I49e18a7a7e1288e3e674e15b6fc86aad60a08214 Gerrit-Change-Number: 21683 Gerrit-PatchSet: 2 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: Thu, 22 Aug 2024 22:09:36 +0000 Gerrit-HasComments: Yes
