Tim Armstrong 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: (5 comments) 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? Done 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@108 PS6, Line 108: pair<string, string> paths; I got confused by which path was which - a bad sign when it's my own code. I introduced some intermediate variables to hopefully make it more readable. 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 leng My reading of the std::string docs are that it should work in either case (reallocating or bumping the trailing part of the string as needed) http://www.cplusplus.com/reference/string/string/replace/ I wrote a quick test program to confirm my understanding, it seems like replacing short with long and long with short both work as expected: https://gist.github.com/timarmstrong/b2e40308310d01faeb4dc1348baf4c58 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: Ah yeah, I missed updating this while working on this code. StringToIntInternal handles it, but I need to treat PARSE_OVERFLOW as a non-error and also validate that it's not negative. 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? Done. Thanks for catching -- 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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Pooja Nilangekar <pooja.nilange...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 04 Jan 2019 00:41:25 +0000 Gerrit-HasComments: Yes
