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 7: (11 comments) I have created another helper method to contain the code for event sequences aggregation, please let me know if I should create the method in any other way instead. http://gerrit.cloudera.org:8080/#/c/21683/6/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: http://gerrit.cloudera.org:8080/#/c/21683/6/be/src/util/runtime-profile.h@940 PS6, Line 940: std::vector<std::vector<int32_t>> label_idxs; : > This will duplicate EventSequenceMap typedef in L786. Maybe rename this int Done http://gerrit.cloudera.org:8080/#/c/21683/6/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/21683/6/be/src/util/runtime-profile.cc@2952 PS6, Line 2952: vector<vector<vector<int64_t>>> timestamps_vec; : // Vector of event labels, addres > Please put comment what each index in vector represent. Done http://gerrit.cloudera.org:8080/#/c/21683/6/be/src/util/runtime-profile.cc@2966 PS6, Line 2966: labels_vec[es_idx].resize(event_sequence_contents.labels.size()); > line too long (95 > 90) Done http://gerrit.cloudera.org:8080/#/c/21683/6/be/src/util/runtime-profile.cc@2968 PS6, Line 2968: // Value : Event label's ordered(actual) index (int32_t) > This does not seem to consult against event_sequence_contents.label_idxs, s I have used key value pairs from 'event_sequence_contents.labels' map for ordering labels instead. After checking against 'AggregatedRuntimeProfile::UpdateEventSequencesFromInstance' method(L542-L552), I found that same values are being assigned for both 'label_idxs' entries and value part of 'labels'. Just to be sure, I had also logged and verified the values to ensure the ordering was same. It seems 'label_idxs' is being used when there is no need to iterating over the map. http://gerrit.cloudera.org:8080/#/c/21683/6/be/src/util/runtime-profile.cc@2985 PS6, Line 2985: EventSequenceToJsonHelper(&event_sequences_json, d > Please put comment right after this line describing what this big branch do Done. I have added comments now along and have also created another helper method. I am not entirely sure, but please do let me know if I should have created the method differently, different parameters or naming... http://gerrit.cloudera.org:8080/#/c/21683/6/be/src/util/runtime-profile.cc@2988 PS6, Line 2988: parent->AddMember("event_sequences", event_sequences_json, allocator); : } > Please rename to min_ts and max_ts to avoid confusion with min and max func Done http://gerrit.cloudera.org:8080/#/c/21683/6/be/src/util/runtime-profile.cc@3021 PS6, Line 3021: min_ts_list.assign(BUCKET_SIZE, max_ts); > Put comment on why this branch is needed. I assume this is to anticipate fo It is in case there are no instances in that division, zeros are being filled to ensure the there are always 5 divisions. This makes it easier to interpret the values while parsing. http://gerrit.cloudera.org:8080/#/c/21683/6/be/src/util/runtime-profile.cc@3034 PS6, Line 3034: inst_count_list[division_idx] += 1; : } > Move this below, after L3040. Done http://gerrit.cloudera.org:8080/#/c/21683/6/tests/observability/test_profile_tool.py File tests/observability/test_profile_tool.py: http://gerrit.cloudera.org:8080/#/c/21683/6/tests/observability/test_profile_tool.py@76 PS6, Line 76: > flake8: E501 line too long (92 > 90 characters) Done http://gerrit.cloudera.org:8080/#/c/21683/6/tests/observability/test_profile_tool.py@79 PS6, Line 79: > flake8: E501 line too long (99 > 90 characters) Done http://gerrit.cloudera.org:8080/#/c/21683/6/tests/observability/test_profile_tool.py@84 PS6, Line 84: > You can replace the string with variable to workaround the "line to long" i Instead of adding "profile_v2" as postfix, I just added "v2" now. So, the statements are smaller and similar to other test functions now. -- 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: 7 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, 23 Oct 2024 22:26:42 +0000 Gerrit-HasComments: Yes
