Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/12120 )
Change subject: IMPALA-7941: part 1: detect cgroups memory limit ...................................................................... Patch Set 6: (4 comments) Looks good. I just have a few questions. http://gerrit.cloudera.org:8080/#/c/12120/6/be/src/util/cgroup-util.h File be/src/util/cgroup-util.h: http://gerrit.cloudera.org:8080/#/c/12120/6/be/src/util/cgroup-util.h@43 PS6, Line 43: is a Nit: extra words? http://gerrit.cloudera.org:8080/#/c/12120/6/be/src/util/cgroup-util.cc File be/src/util/cgroup-util.cc: http://gerrit.cloudera.org:8080/#/c/12120/6/be/src/util/cgroup-util.cc@111 PS6, Line 111: paths.second.size(), paths.first I am probably missing something here, would this cause an issue if the length of the relative path string is lesser than the length of the mount point string? Could we end up with an incomplete string? http://gerrit.cloudera.org:8080/#/c/12120/6/be/src/util/cgroup-util.cc@133 PS6, Line 133: *bytes = static_cast<int64_t>(min<size_t>(v, numeric_limits<int64_t>::max())); Is this necessary? StringParse::StringToIntInternal caps the value at std::numeric_limits<T>::max() http://gerrit.cloudera.org:8080/#/c/12120/5/be/src/util/process-state-info.h File be/src/util/process-state-info.h: http://gerrit.cloudera.org:8080/#/c/12120/5/be/src/util/process-state-info.h@30 PS5, Line 30: Cgroup Nit: Remove? -- To view, visit http://gerrit.cloudera.org:8080/12120 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic0f5ed0e94511361bf5605822c0874c16c45ffa9 Gerrit-Change-Number: 12120 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Pooja Nilangekar <[email protected]> Gerrit-Comment-Date: Thu, 03 Jan 2019 23:20:47 +0000 Gerrit-HasComments: Yes
