Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5658: addtl. process/system-wide memory metrics ......................................................................
Patch Set 2: (3 comments) 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; : // 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 which seems OK but may still be a bit confusing. What do you think of using -1 to indicate no value? Another alternative is for the fn that returns this to return an error if parsing fails, and then not creating the metrics in those cases. Tools like CM should handle the lack of metrics gracefully, i.e. there is no data. Line 59: class MemInfo { in general: it'd be good to test this on ubuntu and sles, at least to make sure the failure case is as graceful as we think it is 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 -- 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-HasComments: Yes
