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 1: (4 comments) Thank you for working on this http://gerrit.cloudera.org:8080/#/c/23950/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23950/1//COMMIT_MSG@22 PS1, Line 22: TODO (for future patches): : - Add cache sizes (L1, L2, L3) information : - Add NUMA node details : - Add memory capacity and type information : - More detailed CPU information (clock speed, : CPU flags like AVX, AVX2, etc.) > TODOs maybe better to put in JIRAs or comments within the codebase at appro I agree with Surya that we don't need to list the future work in detail. Sometimes, we mention general direction of future work like "In future, this can be extended to other host information like cache sizes, etc." but we have other places to track the actual work items. Sometimes, work is left to a follow-up JIRA and a commit message will mention that JIRA. We use that sparingly. The audience is mainly future readers of the commit message, so it is about what they will find useful. http://gerrit.cloudera.org:8080/#/c/23950/1/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/23950/1/be/src/runtime/coordinator.cc@1447 PS1, Line 1447: // TODO(IMPALA-8126): Move to host profiles > This TODO seems to mention that we should move this to per node profiles if Let's keep it separate. It isn't really related to what this JIRA is about. http://gerrit.cloudera.org:8080/#/c/23950/1/be/src/runtime/coordinator.cc@1453 PS1, Line 1453: // Add hardware and OS information > As Joe had mentioned in the JIRA, it's definitely worth aggregating here fo I'm ok with adding OS / CPU to the per node profile. For future host information, I might want to avoid some of the more verbose things to avoid bloating the profile output. http://gerrit.cloudera.org:8080/#/c/23950/1/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/23950/1/be/src/service/impala-server.cc@2678 PS1, Line 2678: be_desc->set_cpu_model_name(CpuInfo::model_name()); : be_desc->set_cpu_num_cores(CpuInfo::num_cores()); : be_desc->set_os_distribution(OsInfo::os_distribution()); Grouping these fields into their own struct would help keep the code clean, especially if we will add more fields: 1. The code in Scheduler::ComputeBackendExecParams() can pass the struct along. Adding a new field wouldn't require that code to change. 2. The code to aggregate the structs could be abstracted out as an aggregator class. Coordinator::ComputeQuerySummary() code can initialize the aggregator class before the loop, call Merge() on each struct as it loops over the backends, then pull information out of the aggregator to put in the profile. (Coordinator::ResourceUtilization does an aggregation like this.) Depending on how we define the API, this may allow the Coordinator::ComputeQuerySummary() code to stay the same as we add fields to the struct. 3. If there is an aggregator class that is somewhat self-contained, then it can be unit tested with hand built structs (including cases with mismatching values). This may be overkill when it is just a couple fields, but it does make it substantially easier to test. -- 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: 1 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, 25 Feb 2026 01:54:55 +0000 Gerrit-HasComments: Yes
