You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Joe McDonnell (Code Review)" <ge...@cloudera.org> on 2026/03/11 20:37:24 UTC
[Impala-ASF-CR] IMPALA-12191: Add hardware and OS details to runtime profile
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 <ar...@gmail.com>
Gerrit-Reviewer: Arnab Karmakar <ar...@gmail.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Surya Hebbar <sh...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Mar 2026 20:37:24 +0000
Gerrit-HasComments: Yes