Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5658: addtl. process/system-wide memory metrics ......................................................................
Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/7472/3/be/src/common/init.cc File be/src/common/init.cc: PS3, Line 132: . nit extra . http://gerrit.cloudera.org:8080/#/c/7472/3/be/src/util/mem-info.cc File be/src/util/mem-info.cc: PS3, Line 132: trim(value); : : if (name == "Size") { : result.size_kb += atol(value.c_str()); : } else if (name == "Rss") { : result.rss_kb += atol(value.c_str()); : } else if (name == "AnonHugePages") { : result.anon_huge_pages_kb += atol(value.c_str()); : } I'm confused why this works for "Size: 1084 kB" - wouldn't the "kB" still be in the string, then atol would fail? I'd expect trim() to behave like " 1084 kB " -> "1084 kB", but maybe it is removing the " kB" as well? http://gerrit.cloudera.org:8080/#/c/7472/2/be/src/util/mem-info.h File be/src/util/mem-info.h: 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; > I don't think we can handle all possible failure modes without complicating I agree testing will be hard, and probably this is fine, but that doesn't mean it'll be consistent across kernel versions. fwiw I found a centos5.7 box, and taking a look at some process shows no anon huge pages. I guess it's OK here since that OS won't even support THP AFAIK, but I'm always skeptical of trusting some expected format in /proc . 40000000-40009000 r-xp 00000000 08:01 198681 /var/lib/jenkins/jdk/bin/java Size: 36 kB Rss: 36 kB Shared_Clean: 0 kB Shared_Dirty: 0 kB Private_Clean: 36 kB Private_Dirty: 0 kB Swap: 0 kB Pss: 36 kB -- 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: 3 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
