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 7: (4 comments) Fixed a few things. Have a follow-on Q about the escapes http://gerrit.cloudera.org:8080/#/c/12120/7/be/src/util/cgroup-util.cc File be/src/util/cgroup-util.cc: http://gerrit.cloudera.org:8080/#/c/12120/7/be/src/util/cgroup-util.cc@40 PS7, Line 40: cgroup_type Per discussion, subsystem is a more accurate term. http://gerrit.cloudera.org:8080/#/c/12120/7/be/src/util/cgroup-util.cc@86 PS7, Line 86: split(fields, line, is_any_of(" "), token_compress_on); > i was curious as to how this will work if the mount point had spaces in it. This still isn't likely to work correctly since we'd need to decode it to get a valid path. http://gerrit.cloudera.org:8080/#/c/12120/7/be/src/util/cgroup-util.cc@86 PS7, Line 86: split(fields, line, is_any_of(" "), token_compress_on); > i was curious as to how this will work if the mount point had spaces in it. Can you send me a pointer to the info you found about this? It seems like we should, in principle, unescape the string so that later operations on it will work but I wanted to confirm the format. gutil has some utilities like CUnescape() that might be good enough for this. It does seem kinda like asking for trouble if someone configures things so that there are spaces in these paths though. http://gerrit.cloudera.org:8080/#/c/12120/7/be/src/util/cgroup-util.cc@112 PS7, Line 112: path->compare(0, system_path.size(), system_path) == 0 > is there a case where this might not be true? Erm, that's a good point. It would be weird if it didn't. I'll return an error if this isn't the case. -- 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: 7 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@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, 11 Jan 2019 23:39:29 +0000 Gerrit-HasComments: Yes
