Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/23154 )
Change subject: IMPALA-9846: Enable Aggregated Runtime Profile by Default ...................................................................... Patch Set 21: (2 comments) http://gerrit.cloudera.org:8080/#/c/23154/13//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23154/13//COMMIT_MSG@16 PS13, Line 16: are substan > But, separating 1 and 2 would not be possible. I didn't understand this at first, but now I think I get it - my assumption was that only the text/json format of aggregated profiles was changed, but I just realized that non-aggregated profiles were also changed by adding "total" to averaged fragments. Changing existing (non aggregated) text profiles is something I disagree with unless there is a good reason to do so. The same format is used since many years and I know some people (including myself) who have scripts / one liners to extract info from text profiles. This does not seem to be a problem for aggregated profiles, which were considered experimental before this patch. Is the reason for changing the old profile is to help with keeping the regexps in EE tests working with both old and aggregated profiles? I don't see this as something critical so I disagree with changing the old format just to achieve this ( see my comment on https://gerrit.cloudera.org/#/c/23154/13/tests/common/test_result_verifier.py@626 ) http://gerrit.cloudera.org:8080/#/c/23154/13/tests/common/test_result_verifier.py File tests/common/test_result_verifier.py: http://gerrit.cloudera.org:8080/#/c/23154/13/tests/common/test_result_verifier.py@624 PS13, Line 624: # <field_name>: val.(<value>) ... : # : # Note: Fields witho > I was told to mantain support for both aggregated and unaggregated profiles >I was told to mantain support for both aggregated and unaggregated profiles Can you clarify what supporting both would mean? The change does not remove old profile code, so it should keep working, but tests will run with aggregated profiles, so there will be only minimal coverage for old profiles. I don't see much benefit in keeping the EE tests runnable with old profiles if we don't actually run these tests with old profiles. -- To view, visit http://gerrit.cloudera.org:8080/23154 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If41d6322361fba82c946efd614cc7d28cb1c36e8 Gerrit-Change-Number: 23154 Gerrit-PatchSet: 21 Gerrit-Owner: Surya Hebbar <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[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, 11 Nov 2025 14:15:22 +0000 Gerrit-HasComments: Yes
