Tim Armstrong has posted comments on this change. Change subject: IMPALA-5658: addtl. process/system-wide memory metrics ......................................................................
Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/7472/2/be/src/common/init.cc File be/src/common/init.cc: Line 148: AggregateMemoryMetric::Refresh(); > This won't get refreshed on the statestore or catalog service because of li I think this should still happen after Maintenance() since that can free up memory. Changed the flow so this executes even if ExecEnv is not present. http://gerrit.cloudera.org:8080/#/c/7472/2/be/src/util/mem-info.h File be/src/util/mem-info.h: Line 32: // Total size of memory maps (i.e. virtual memory size) in kilobytes. > nit: blank line before comments makes it easier to read. Done PS2, Line 29: struct MappedMemInfo { : // Number of memory maps. : int64_t num_maps = 0; : // Total size of memory maps (i.e. virtual memory size) in kilobytes. : int64_t size_kb = 0; : // RSS in kilobytes : int64_t rss_kb = 0; : // Kilobytes of anonymous huge pages. : int64_t anon_huge_pages_kb = 0; > So if these can't be parsed, we'll end up writing 0 for all of the metrics I don't think we can handle all possible failure modes without complicating the parsing code with additional validations, e.g. checking if the expected lines are present for each map. From the docs it looks like all of these lines should be present if /smaps is present. /smaps can be disabled by a kernel config parameter though. http://man7.org/linux/man-pages/man5/proc.5.html I changed it to avoid creating the metrics if the /smaps file isn't present. I don't think any more granular validation would be testable. I tested the change manually by changing the string to look for /ssmaps, which doesn't exist. Line 59: class MemInfo { > in general: I'm testing locally on Ubuntu, plus jenkins.impala.io runs on Ubuntu mostly. I kicked off a test run on a RHEL6 system I have access to too. AFAIK all the distributions should expose the same interfaces unless they are running kernels that predate those features or have them disabled. The only exception I know of is that some older versions of RHEL have /sys/kernel/mm/redhat_transparent_hugepage instead of /sys/kernel/mm/transparent_hugepage. I checked CentOS 6.6 and it has a symlink from the second to the first, so it may only be 6.5 and earlier. http://gerrit.cloudera.org:8080/#/c/7472/2/be/src/util/memory-metrics.cc File be/src/util/memory-metrics.cc: PS2, Line 44: NULL > update these to nullptr if you don't mind while you're here Done http://gerrit.cloudera.org:8080/#/c/7472/2/be/src/util/memory-metrics.h File be/src/util/memory-metrics.h: PS2, Line 40: AggregateMemoryMetric > is it a big pain to rename this AggregateMemoryMetrics ? The name right now Done PS2, Line 77: value values > typo Done -- To view, visit http://gerrit.cloudera.org:8080/7472 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I13873e305ba464d11dea0d7244a29ff4f332f1a9 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
