Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21683 )
Change subject: IMPALA-13304: Include aggregate instance-level metrics in JSON profile ...................................................................... Patch Set 17: (9 comments) http://gerrit.cloudera.org:8080/#/c/21683/17/be/src/util/runtime-profile-test.cc File be/src/util/runtime-profile-test.cc: http://gerrit.cloudera.org:8080/#/c/21683/17/be/src/util/runtime-profile-test.cc@1961 PS17, Line 1961: NUM_PROFILES = uni_dis_instances(gen); VLOG(1) or cout the randomized NUM_PROFILES and BUCKET_SIZE. http://gerrit.cloudera.org:8080/#/c/21683/17/be/src/util/runtime-profile-test.cc@1967 PS17, Line 1967: greater than greater than or equal to? uniform_int_distribution range is inclusive, right? http://gerrit.cloudera.org:8080/#/c/21683/17/be/src/util/runtime-profile-test.cc@1970 PS17, Line 1970: BUCKET_SIZE = uni_dis_granularity(gen); : // To distribute the case of bucket size = 0 with equal probability : BUCKET_SIZE = BUCKET_SIZE == NUM_PROFILES + 1 ? 0 : BUCKET_SIZE; I don't really like this. The randomization may never pick BUCKET_SIZE == NUM_PROFILES + 1. It is better to add 3rd parameter, bool is_zero_bucket_size, and combine it with timestamp_aggregation=false. http://gerrit.cloudera.org:8080/#/c/21683/17/be/src/util/runtime-profile-test.cc@2015 PS17, Line 2015: dummy_event_duration > This should have been 1ms. Do not want to trigger another unnecessary build Go ahead and fix it in next patch set. http://gerrit.cloudera.org:8080/#/c/21683/17/be/src/util/runtime-profile-test.cc@2251 PS17, Line 2251: GroupedCase NonAggregatedCase? http://gerrit.cloudera.org:8080/#/c/21683/17/be/src/util/runtime-profile-test.cc@2253 PS17, Line 2253: bucket size <= number Should this be "bucket_size >= number"? http://gerrit.cloudera.org:8080/#/c/21683/14/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/21683/14/be/src/util/runtime-profile.cc@66 PS14, Line 66: json_profile_event_timestamp_limit, 5, > I think this might be a good query option in the future as it would affect Query option might not fit well for this. Imagine that I ran a query using impala-shell, and then close the shell. Then I open profile through WebUI. The context of query option when I run the query is already gone after I close impala-shell. Flag is also easily applicable to impala-profile-tool. Maybe, URL option is what you want here. Let say, If I want a bucket of 5, I can open in browser: http://localhost:25000/query_profile?query_id=224b6ebc6755a67c:3d124ee400000000&num_event_group=5 Ofcourse, the coordinator must start with --gen_experimental_profile=true flag. We can try implement that as separate patch after this. http://gerrit.cloudera.org:8080/#/c/21683/14/be/src/util/runtime-profile.cc@3037 PS14, Line 3037: timestamps[instance_idx] = vector<int64_t>(events_count); : const vector<int32_t>& idxs = label_idxs[instance_idx]; : int32_t inst_event_count = idxs.size(); > Done. To reduce the checks each time, but still to make it clearer to under Ack http://gerrit.cloudera.org:8080/#/c/21683/17/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/21683/17/be/src/util/runtime-profile.cc@67 PS17, Line 67: "Sets the maximum number of instance timestamps to be included within the JSON " : "profile's aggregated event sequence, above which they are bucketed into the set " : "number of aggregates. A value of zero disables aggregation." Description can be better here. Example can also help. How about: "Sets the number of group to aggregate event sequence timestamps of same name from all " "fragment instances in JSON profile. For example, if value is M and total fragment " "instances is N, and M < N, then all N event sequences of same name will be presented " "in M groups. This flag is ignored if M = 0, M >= N, or --gen_experimental_profile=false." -- 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: 17 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: Wed, 29 Jan 2025 00:18:43 +0000 Gerrit-HasComments: Yes
