Surya Hebbar has posted comments on this change. ( http://gerrit.cloudera.org:8080/24576 )
Change subject: IMPALA-15174: Add AGGREGATED_PROFILE query option to toggle profile formats ...................................................................... Patch Set 3: (16 comments) Thank you for the review. I've fixed the suggested issues. For profile_version, I have left an explanation in the comments and asked for your advice. http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.h@310 PS2, Line 310: aggregated_profile > members should end with _ Done http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.h@538 PS2, Line 538: um_instances_; > members should end with _ Done http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.cc@61 PS2, Line 61: (backend_client_rp > This doesn't seem to be used in the file Done http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.cc@515 PS2, Line 515: aggregated_profile_ || backend_exec_status.frag > Could use a local "bool aggregated_profile" for this, it is used at several Done http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.cc@517 PS2, Line 517: ing it half-way th > this indicates flag instead of query option Done http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.cc@533 PS2, Line 533: or per fragment in > same as line 517 Done http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.cc@573 PS2, Line 573: > line too long (99 > 90) Done http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.cc@636 PS2, Line 636: nstances( > Is it still experimental? Done http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.cc@692 PS2, Line 692: } > line too long (94 > 90) Done http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator.cc@84 PS2, Line 84: DECLARE_string(hostname); > seems unused Done http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator.cc@256 PS2, Line 256: } : } > I don't understand this, why it is added in a loop that iterates through fr This was added inside by mistake. I completely missed the loop's closing bracket and did not take a second look here after verifying the position of the info string in output. It was being updated/re-added unnecessarily. I have fixed it now. Thank you. http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/service/impala-server.cc@997 PS2, Line 997: tmp_profile->ToJson(verbosity, profile->json_output); > Can't QueryStateRecord have this information? Its constructors get ClientRe I was trying to avoid repeating query profile's attributes in query state record. But, I see now that attributes like default_db, stmt etc are also present in query state record. I have added aggregated_profile as a new member now. Thank you. http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/service/impala-server.cc@1522 PS2, Line 1522: // thread-safe). > line too long (102 > 90) Done http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/service/impala-server.cc@2186 PS2, Line 2186: CLEAN_EXIT_WITH_ERROR(Substitute("Invalid default query options. Please > Is it useful to have both flag and query option? Users could override in FL Yes, as an important option, it might be worth it to have an extra flag like before. Also, I agree that it does help avoid the noise. So, I am leaning towards having both flag and the query option. But, I am fine with taking either approach and will follow your recommendations. http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/util/impala-profile-tool.cc File be/src/util/impala-profile-tool.cc: http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/util/impala-profile-tool.cc@a138 PS2, Line 138: > Why is this no longer used? I noticed that profile_version didn't seem to be used anywhere meaningfully; it was essentially just being appended based on the aggregated or unaggregated profile flag. Given that, it didn't seem reasonable for the same Impala version to output different profile version numbers based solely on those flags. Additionally, I’m unsure what the exact criteria are for defining a "new" profile version, and whether the session-based profile type attribute is already sufficient for this purpose. Could you let me know if my assumption here is incorrect? Specifically, is profile_version being relied upon for inference by external tools like Impyla? My understanding was that they do not use it. If we do need to keep and update this versioning, could you suggest the criteria I should use to determine a version bump, and where in the code it would be most appropriate to implement that logic? http://gerrit.cloudera.org:8080/#/c/24576/2/tests/custom_cluster/test_observability.py File tests/custom_cluster/test_observability.py: http://gerrit.cloudera.org:8080/#/c/24576/2/tests/custom_cluster/test_observability.py@100 PS2, Line 100: ggregated_profile > This could be moved out from this suite to a non-custom cluster test (proba Ack -- To view, visit http://gerrit.cloudera.org:8080/24576 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I96aceda1b044ae53d09e68cb78678fc2930326dd Gerrit-Change-Number: 24576 Gerrit-PatchSet: 3 Gerrit-Owner: Surya Hebbar <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Mon, 27 Jul 2026 08:18:48 +0000 Gerrit-HasComments: Yes
