Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/10277 )
Change subject: IMPALA-5737: Tighten minicluster memory limit ...................................................................... Patch Set 2: (6 comments) Sorry for the delay; I missed that you'd updated this. Feel free to ping me sooner in the future! My stylistic nit is that it would be better if the variables had the unit in their names, so renaming everything to ..._MB. http://gerrit.cloudera.org:8080/#/c/10277/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10277/2//COMMIT_MSG@9 PS2, Line 9: This patch limits minicluster memory based on the memory avaliable. available http://gerrit.cloudera.org:8080/#/c/10277/2/bin/impala-config.sh File bin/impala-config.sh: http://gerrit.cloudera.org:8080/#/c/10277/2/bin/impala-config.sh@634 PS2, Line 634: if [ -z "${MINI_CLUSTER_MEM_LIMIT+x}" ]; then What's the "+x" doing here? I looked up what this was doing, but it may be worthwhile to add a comment like: # If MINI_CLUSTER_MEM_LIMIT isn't set, configure it based on system memory. In other places, we divide this sort of thing into a X_OVERRIDE and X variable to avoid issues when jumping between source directories, but it probably doesn't matter too much here. Please consider renaming this to MINI_CLUSTER_MEM_LIMIT_MB so that it's obvious what unit we're talking about. http://gerrit.cloudera.org:8080/#/c/10277/2/bin/impala-config.sh@636 PS2, Line 636: $(($(cat /proc/meminfo | grep -oP "MemAvailable:[[:space:]]*\K\d*")/1024)) I did not know about \K! http://gerrit.cloudera.org:8080/#/c/10277/2/bin/impala-config.sh@639 PS2, Line 639: CGROUP_LIMIT=$(cat /sys/fs/cgroup/memory/memory.limit_in_bytes) This is the current process's cgroup? Pointer to how this works? Technically cgroups could be mounted in a different location; what environments does this work in? http://gerrit.cloudera.org:8080/#/c/10277/2/bin/impala-config.sh@654 PS2, Line 654: export IMPALAD_MEM_LIMIT=6144 Ideally all these names have units like _MB appended to them. http://gerrit.cloudera.org:8080/#/c/10277/2/bin/start-impala-cluster.py File bin/start-impala-cluster.py: http://gerrit.cloudera.org:8080/#/c/10277/2/bin/start-impala-cluster.py@232 PS2, Line 232: env_mem_limit = os.environ.get('IMPALAD_MEM_LIMIT') It's somewhat problematic that we now have multiple ways to do: export TEST_START_CLUSTER_ARGS="--impalad_args=--mem_limit=$IMPALAD_MEM_LIMIT_BYTES" On balance, I think this is fine. Variables like TEST_START_CLUSTER_ARGS are really arrays, and it's hard to figure out a good way to consolidate them. -- To view, visit http://gerrit.cloudera.org:8080/10277 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If8240551e726c6da546a926a1ce3444f41ef87fe Gerrit-Change-Number: 10277 Gerrit-PatchSet: 2 Gerrit-Owner: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Comment-Date: Thu, 17 May 2018 00:41:00 +0000 Gerrit-HasComments: Yes
