impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5658: addtl. process/system-wide memory metrics
Date Mon, 24 Jul 2017 20:35:46 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message