Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/23950 )
Change subject: IMPALA-12191: Add hardware and OS details to runtime profile ...................................................................... Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/23950/4/be/src/runtime/backend-hardware-info.h File be/src/runtime/backend-hardware-info.h: http://gerrit.cloudera.org:8080/#/c/23950/4/be/src/runtime/backend-hardware-info.h@29 PS4, Line 29: BackendHardwareInfo Since we include OS settings, can we change this to BackendMachineInfo? http://gerrit.cloudera.org:8080/#/c/23950/4/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/23950/4/be/src/runtime/coordinator.cc@1442 PS4, Line 1442: // Add aggregated hardware and OS information : if (hw_info_aggregator.HasData()) { : string cpu_summary = hw_info_aggregator.GetCpuSummary(); : if (!cpu_summary.empty()) { : query_profile_->AddInfoString("Backend CPU Info", cpu_summary); : } : : string os_summary = hw_info_aggregator.GetOsSummary(); : if (!os_summary.empty()) { : query_profile_->AddInfoString("Backend OS Info", os_summary); : } : } This is ok for now. At some point when we have more fields, we may want the BackendHardwareAggregator to return a map with key/value pairs. This would simply add those to the query_profile_ without needing to know about each one. http://gerrit.cloudera.org:8080/#/c/23950/4/common/protobuf/statestore_service.proto File common/protobuf/statestore_service.proto: http://gerrit.cloudera.org:8080/#/c/23950/4/common/protobuf/statestore_service.proto@94 PS4, Line 94: // CPU model name : optional string cpu_model_name = 16; : : // Number of CPU cores available on this backend : optional int32 cpu_num_cores = 17; : : // OS distribution name : optional string os_distribution = 18; One small thing: Let's go ahead and turn this into its own struct / message. The constructor for BackendHardwareInfo would take this in rather than the full BackendDescriptorPB. The information can be a single field for BackendExecParamsPB and the code in scheduler can set a single field that won't change when we add more information. I think statestore_server.proto is the right place to define it, because admission_control_service.proto includes it and can use the struct. The code for BackendHardwareInfo is basically identical to what this struct would be, so we may not need that struct if we add this one. -- To view, visit http://gerrit.cloudera.org:8080/23950 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib5a093b6cf9dd7dfa4321b79aeea95d6fc748dd4 Gerrit-Change-Number: 23950 Gerrit-PatchSet: 4 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: David Rorke <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Wed, 11 Mar 2026 20:37:24 +0000 Gerrit-HasComments: Yes
