Surya Hebbar 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 3: (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 5 > Is it suppose to be 5 or 4? With the new logic, it is 5 now. http://gerrit.cloudera.org:8080/#/c/21683/2//COMMIT_MSG@37 PS2, Line 37: assignment to a particular > I rather be consistent with displaying this in group-of-4 formats for any c The new display mechanism requires 5 divisions now. Many times there are no timestamps in a division, and value will be 0 instead. The length will always be 5 now. http://gerrit.cloudera.org:8080/#/c/21683/2//COMMIT_MSG@80 PS2, Line 80: num_children : <NUM_CHILDREN> : node_metadata : <NODE_METADATA_OBJECT> : event_sequences : > Please run tests/observability/test_profile_tool.py with this patch. The te Once the JSON's structure is refined after some reviews, I will add the tests and correct the expected JSONs. 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? I had done it for consistency. I will remove it in the next patchset, if not necessary. 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? It mentioned that "the per-instance" values are not included. But as event sequences are included this was not correct. I have rewrote it now, and I will make necessary modifications according to suggestions. 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 5, aggregate of event timestamps (i.e. Average, : // Minimum and Maximum) is included after bucketing the instance timestamps into : // 5 divisions each spanning 20%. > Put "Event sequence" title, like SummaryStatsCounter comment before this. To be consistent with the 'ToJson' method for the traditional profile above, I hae commented "2. Events" instead. Should I change both comments to "Event sequences"? http://gerrit.cloudera.org:8080/#/c/21683/2/be/src/util/runtime-profile.cc@2957 PS2, Line 2957: t_sequence > Create a descriptive variable name to refer e_s.second and use it everywher Done http://gerrit.cloudera.org:8080/#/c/21683/2/be/src/util/runtime-profile.cc@2963 PS2, Line 2963: > int should be fine here and others below. Done. I have used size_t instead now. http://gerrit.cloudera.org:8080/#/c/21683/2/be/src/util/runtime-profile.cc@2968 PS2, Line 2968: Value event(kObjectType); > Move this before L2957 and refer to events_count variable anywhere else. Done http://gerrit.cloudera.org:8080/#/c/21683/2/be/src/util/runtime-profile.cc@2979 PS2, Line 2979: if (cur > max) max = cur; : } : int64_t ev_ts_span = max - min; : vector<int64_t> min_ts_list(BUCKET_SIZE, max); > Some of the iterator variable here can be made clearer with more descriptiv Done. I have used division_idx instead of grouping_idx, as the aggregated metrics logic has changed. Otherwise, should I use span_idx or something similar to that? -- 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: 3 Gerrit-Owner: Surya Hebbar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Tue, 17 Sep 2024 21:08:45 +0000 Gerrit-HasComments: Yes
