You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "mddxhj (via GitHub)" <gi...@apache.org> on 2023/09/08 11:41:58 UTC
[GitHub] [incubator-celeborn] mddxhj opened a new pull request, #1824: [CELEBORN-900] Prefer to use jemalloc for memory allocation
mddxhj opened a new pull request, #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824
### What changes were proposed in this pull request?
Only the Dockfile needs to change in this pr.
### Why are the changes needed?
When deploying celeborn for flink on kubernetes, Introducing jemalloc can improve pod memory usage.
### Does this PR introduce _any_ user-facing change?
None
### How was this patch tested?
Maybe starting a production job to test the memory usage improvement is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] Myasuka commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "Myasuka (via GitHub)" <gi...@apache.org>.
Myasuka commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1301311109
##########
docker/Dockerfile:
##########
@@ -35,6 +35,8 @@ RUN set -ex && \
useradd --uid=${celeborn_uid} --gid=${celeborn_gid} celeborn -d /home/celeborn -m && \
mkdir -p ${CELEBORN_HOME}
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
Review Comment:
Thanks for the clarification. Reconfigure the environment `LD_PRELOAD` in script would align the behavior. However, once running outside the container, the location of `libjemalloc.so` would be totally different in different versions of linux environment. To not extend this scope of this PR (mainly focus on the container environment), I think we would not pay too much efforts to find the location of `libjemalloc.so`. The improvement could be done for others who runn Celeborn in non-container environments.
In a nutshell, I think moving the setting of `LD_PRELOAD` to environment related scripts would be a good idea, but we will only detect the `/usr/lib/$(uname -m)-linux-gnu/libjemalloc.so` location in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#issuecomment-1711225088
@mddxhj thanks for updating, left some comments. and please update the PR title too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] mddxhj commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "mddxhj (via GitHub)" <gi...@apache.org>.
mddxhj commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1301476575
##########
docker/Dockerfile:
##########
@@ -35,6 +35,8 @@ RUN set -ex && \
useradd --uid=${celeborn_uid} --gid=${celeborn_gid} celeborn -d /home/celeborn -m && \
mkdir -p ${CELEBORN_HOME}
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
Review Comment:
@pan3793 I think there's no other concerns，I'll try to migrate LD_PRELOAD configuration to load-celeborn-env.sh。
##########
docker/Dockerfile:
##########
@@ -35,6 +35,8 @@ RUN set -ex && \
useradd --uid=${celeborn_uid} --gid=${celeborn_gid} celeborn -d /home/celeborn -m && \
mkdir -p ${CELEBORN_HOME}
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
Review Comment:
@pan3793 I think there's no other concerns，I'll try to migrate LD_PRELOAD configuration to load-celeborn-env.sh。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Prefer to use jemalloc for memory allocation
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1319725336
##########
sbin/load-celeborn-env.sh:
##########
@@ -69,3 +69,24 @@ if [ "$CELEBORN_PID_DIR" = "" ]; then
export CELEBORN_PID_DIR="${CELEBORN_HOME}/pids"
fi
+# jemalloc memory allocator is enabled by default, you may set DISABLE_JEMALLOC to true in celeborn-env.sh if it's not as you wish.
+maybe_enable_jemalloc() {
+ if [ "${DISABLE_JEMALLOC:-false}" == "false" ]; then
+ JEMALLOC_PATH="/usr/lib/$(uname -m)-linux-gnu/libjemalloc.so"
+ JEMALLOC_FALLBACK="/usr/lib/x86_64-linux-gnu/libjemalloc.so"
+ if [ -f "$JEMALLOC_PATH" ]; then
+ export LD_PRELOAD=$LD_PRELOAD:$JEMALLOC_PATH
+ elif [ -f "$JEMALLOC_FALLBACK" ]; then
+ export LD_PRELOAD=$LD_PRELOAD:$JEMALLOC_FALLBACK
+ else
+ if [ "$JEMALLOC_PATH" == "$JEMALLOC_FALLBACK" ]; then
+ MSG_PATH=$JEMALLOC_PATH
Review Comment:
```suggestion
MSG_PATH="$JEMALLOC_PATH"
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1319487510
##########
sbin/load-celeborn-env.sh:
##########
@@ -69,3 +69,25 @@ if [ "$CELEBORN_PID_DIR" = "" ]; then
export CELEBORN_PID_DIR="${CELEBORN_HOME}/pids"
fi
+# jemalloc memory allocator is enabled by default, you may set DISABLE_JEMALLOC to true if it's not as you wish.
+DISABLE_JEMALLOC="false"
+maybe_enable_jemalloc() {
+ if [ "${DISABLE_JEMALLOC:-false}" == "false" ]; then
Review Comment:
please adding the configurable env vars into the `celeborn-env.sh.template`
##########
sbin/load-celeborn-env.sh:
##########
@@ -69,3 +69,25 @@ if [ "$CELEBORN_PID_DIR" = "" ]; then
export CELEBORN_PID_DIR="${CELEBORN_HOME}/pids"
fi
+# jemalloc memory allocator is enabled by default, you may set DISABLE_JEMALLOC to true if it's not as you wish.
+DISABLE_JEMALLOC="false"
+maybe_enable_jemalloc() {
+ if [ "${DISABLE_JEMALLOC:-false}" == "false" ]; then
Review Comment:
please add the configurable env vars into the `celeborn-env.sh.template`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Prefer to use jemalloc for memory allocation
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1319717968
##########
conf/celeborn-env.sh.template:
##########
@@ -31,4 +31,5 @@
# CELEBORN_WORKER_JAVA_OPTS="-XX:-PrintGC -XX:+PrintGCDetails -XX:+PrintGCTimeStamps -XX:+PrintGCDateStamps -Xloggc:gc-worker.out -Dio.netty.leakDetectionLevel=advanced"
# CELEBORN_MASTER_JAVA_OPTS="-XX:-PrintGC -XX:+PrintGCDetails -XX:+PrintGCTimeStamps -XX:+PrintGCDateStamps -Xloggc:gc-master.out -Dio.netty.leakDetectionLevel=advanced"
# CELEBORN_PID_DIR="$CELEBORN_HOME/pids"
-# CELEBORN_LOG_DIR="$CELEBORN_HOME/logs"
\ No newline at end of file
+# CELEBORN_LOG_DIR="$CELEBORN_HOME/logs"
+DISABLE_JEMALLOC="false"
Review Comment:
```suggestion
# DISABLE_JEMALLOC="false"
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] waitinfuture commented on pull request #1824: [CELEBORN-900] Prefer to use jemalloc for memory allocation
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#issuecomment-1711518081
Merging to main/0.3
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1319482372
##########
sbin/load-celeborn-env.sh:
##########
@@ -69,3 +69,25 @@ if [ "$CELEBORN_PID_DIR" = "" ]; then
export CELEBORN_PID_DIR="${CELEBORN_HOME}/pids"
fi
+# jemalloc memory allocator is enabled by default, you may set DISABLE_JEMALLOC to true if it's not as you wish.
+DISABLE_JEMALLOC="false"
+maybe_enable_jemalloc() {
+ if [ "${DISABLE_JEMALLOC:-false}" == "false" ]; then
Review Comment:
nit: style, two indents
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1301201526
##########
docker/Dockerfile:
##########
@@ -35,6 +35,8 @@ RUN set -ex && \
useradd --uid=${celeborn_uid} --gid=${celeborn_gid} celeborn -d /home/celeborn -m && \
mkdir -p ${CELEBORN_HOME}
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
Review Comment:
this may be a problem for arm64 environments, will it work if we put a hint into `$CELEBORN_HOME/conf/celeborn-env.sh`, it also makes it configurable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#issuecomment-1687597618
@mddxhj please fill in the PR description with the necessary introduction on how it works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1301117017
##########
docker/Dockerfile:
##########
@@ -23,10 +23,11 @@ ARG celeborn_gid=10006
ENV CELEBORN_HOME=/opt/celeborn
ENV PATH=/opt/java/openjdk/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/opt/busybox:${CELEBORN_HOME}/sbin:${CELEBORN_HOME}/bin
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
RUN set -ex && \
apt-get update && \
- apt-get install -y bash tini busybox bind9-utils telnet net-tools procps krb5-user dnsutils && \
+ apt-get install -y bash tini busybox bind9-utils telnet net-tools procps krb5-user dnsutils gpg libsnappy1v5 gettext-base libjemalloc-dev && \
Review Comment:
why `gpg` `libsnappy1v5` `gettext-base` are required?
```
➜ ~ apt depends libjemalloc-dev
libjemalloc-dev
Depends: libjemalloc2 (= 5.2.1-1ubuntu1)
Suggests: binutils
➜ ~ apt depends libjemalloc2
libjemalloc2
Depends: libc6 (>= 2.29)
Depends: libgcc-s1 (>= 3.3)
Depends: libstdc++6 (>= 4.4.0)
```
##########
docker/Dockerfile:
##########
@@ -23,10 +23,11 @@ ARG celeborn_gid=10006
ENV CELEBORN_HOME=/opt/celeborn
ENV PATH=/opt/java/openjdk/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/opt/busybox:${CELEBORN_HOME}/sbin:${CELEBORN_HOME}/bin
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
RUN set -ex && \
apt-get update && \
- apt-get install -y bash tini busybox bind9-utils telnet net-tools procps krb5-user dnsutils && \
+ apt-get install -y bash tini busybox bind9-utils telnet net-tools procps krb5-user dnsutils gpg libsnappy1v5 gettext-base libjemalloc-dev && \
Review Comment:
why `gpg` `libsnappy1v5` `gettext-base` are required?
```
➜ ~ apt depends libjemalloc-dev
libjemalloc-dev
Depends: libjemalloc2 (= 5.2.1-1ubuntu1)
Suggests: binutils
➜ ~ apt depends libjemalloc2
libjemalloc2
Depends: libc6 (>= 2.29)
Depends: libgcc-s1 (>= 3.3)
Depends: libstdc++6 (>= 4.4.0)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] Myasuka commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "Myasuka (via GitHub)" <gi...@apache.org>.
Myasuka commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1301273449
##########
docker/Dockerfile:
##########
@@ -35,6 +35,8 @@ RUN set -ex && \
useradd --uid=${celeborn_uid} --gid=${celeborn_gid} celeborn -d /home/celeborn -m && \
mkdir -p ${CELEBORN_HOME}
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
Review Comment:
+1. I think your proposal is almost the same as what Flink does currently. The `uname -m` would be executed in the `docker-entrypoint.sh` instead of `Dockerfile` building.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Prefer to use jemalloc for memory allocation
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1320185259
##########
sbin/load-celeborn-env.sh:
##########
@@ -69,3 +69,24 @@ if [ "$CELEBORN_PID_DIR" = "" ]; then
export CELEBORN_PID_DIR="${CELEBORN_HOME}/pids"
fi
+# jemalloc memory allocator is enabled by default, you may set DISABLE_JEMALLOC to true in celeborn-env.sh if it's not as you wish.
Review Comment:
sound reasonable. how about
1) make `PREFER_JEMALLOC` and `JEMALLOC_PATH` configurable in `celeborn-env.sh`
2) disable `PREFER_JEMALLOC` by default, at least for `branch-0.3`, we may enable it by default in the future
3) consider the adoption rate of Linux distribution, when `PREFER_JEMALLOC=true` and `JEMALLOC_PATH` is absent, search the default installed path of jemalloc libraries on Debian and CentOS 7
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] Myasuka commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "Myasuka (via GitHub)" <gi...@apache.org>.
Myasuka commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1301222722
##########
docker/Dockerfile:
##########
@@ -35,6 +35,8 @@ RUN set -ex && \
useradd --uid=${celeborn_uid} --gid=${celeborn_gid} celeborn -d /home/celeborn -m && \
mkdir -p ${CELEBORN_HOME}
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
Review Comment:
From my experience, I think we could use code to switch the environment via `uname -m` (we can refer to the [flink-docker code](https://github.com/apache/flink-docker/blob/45c6d230407d89aa83b0d170dd056d6868cf808e/1.17/scala_2.12-java8-ubuntu/docker-entrypoint.sh#L92C3-L109)).
I think it make sense to make this option configurable and we shall enable this by default.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] cfmcgrady commented on pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#issuecomment-1689258304
I believe this PR isn't just about the "flink on k8s", but also aims to enhance memory allocation/utilization when deploying the Celeborn server on k8s.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] Myasuka commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "Myasuka (via GitHub)" <gi...@apache.org>.
Myasuka commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1301385161
##########
docker/Dockerfile:
##########
@@ -23,10 +23,11 @@ ARG celeborn_gid=10006
ENV CELEBORN_HOME=/opt/celeborn
ENV PATH=/opt/java/openjdk/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/opt/busybox:${CELEBORN_HOME}/sbin:${CELEBORN_HOME}/bin
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
RUN set -ex && \
apt-get update && \
- apt-get install -y bash tini busybox bind9-utils telnet net-tools procps krb5-user dnsutils && \
+ apt-get install -y bash tini busybox bind9-utils telnet net-tools procps krb5-user dnsutils gpg libsnappy1v5 gettext-base libjemalloc-dev && \
Review Comment:
`libjemalloc-dev` could help [profiling the memory](https://github.com/jemalloc/jemalloc/wiki/Use-Case%3A-Heap-Profiling) to analysis the native memory leak problem. This is extremely helpful for complex OOMKilled problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] mddxhj commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "mddxhj (via GitHub)" <gi...@apache.org>.
mddxhj commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1319486173
##########
docker/Dockerfile:
##########
@@ -35,6 +35,8 @@ RUN set -ex && \
useradd --uid=${celeborn_uid} --gid=${celeborn_gid} celeborn -d /home/celeborn -m && \
mkdir -p ${CELEBORN_HOME}
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
Review Comment:
> @mddxhj could you make such changes if you don't have further concerns?
@pan3793 I have changed the implementation as mentioned above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Prefer to use jemalloc for memory allocation
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1320185259
##########
sbin/load-celeborn-env.sh:
##########
@@ -69,3 +69,24 @@ if [ "$CELEBORN_PID_DIR" = "" ]; then
export CELEBORN_PID_DIR="${CELEBORN_HOME}/pids"
fi
+# jemalloc memory allocator is enabled by default, you may set DISABLE_JEMALLOC to true in celeborn-env.sh if it's not as you wish.
Review Comment:
sound reasonable. how about
1) make `PREFER_JEMALLOC` and `JEMALLOC_PATH` configurable in `celeborn-env.sh`
2) disable `PREFER_JEMALLOC` by default, at least for `branch-0.3`, we may enable it by default in the future
3) consider the adoption rate of Linux distribution, when `PREFER_JEMALLOC=true` and `JEMALLOC_PATH` is absent, search the default installation path of jemalloc libraries on Debian and CentOS 7
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] codecov[bot] commented on pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#issuecomment-1687653649
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1824?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#1824](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1824?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (2278e4b) into [main](https://app.codecov.io/gh/apache/incubator-celeborn/commit/37e62f7e55f3d3fb187c2c40e1c6aabebb3cefb2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (37e62f7) will **decrease** coverage by `0.15%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #1824 +/- ##
==========================================
- Coverage 46.59% 46.43% -0.15%
==========================================
Files 163 163
Lines 10135 10135
Branches 934 934
==========================================
- Hits 4721 4705 -16
- Misses 5105 5118 +13
- Partials 309 312 +3
```
[see 1 file with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1824/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1319484947
##########
sbin/load-celeborn-env.sh:
##########
@@ -69,3 +69,25 @@ if [ "$CELEBORN_PID_DIR" = "" ]; then
export CELEBORN_PID_DIR="${CELEBORN_HOME}/pids"
fi
+# jemalloc memory allocator is enabled by default, you may set DISABLE_JEMALLOC to true if it's not as you wish.
+DISABLE_JEMALLOC="false"
Review Comment:
I suppose this declaration should be removed? otherwise, it will overwrite the definition in `celeborn-env.sh`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] waitinfuture closed pull request #1824: [CELEBORN-900] Prefer to use jemalloc for memory allocation
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #1824: [CELEBORN-900] Prefer to use jemalloc for memory allocation
URL: https://github.com/apache/incubator-celeborn/pull/1824
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1301201526
##########
docker/Dockerfile:
##########
@@ -35,6 +35,8 @@ RUN set -ex && \
useradd --uid=${celeborn_uid} --gid=${celeborn_gid} celeborn -d /home/celeborn -m && \
mkdir -p ${CELEBORN_HOME}
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
Review Comment:
this may be a problem for arm64 environments, will it work if we put a hint into `$CELEBORN_HOME/conf/celeborn-env.sh`? it also makes it configurable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1301205678
##########
docker/Dockerfile:
##########
@@ -23,10 +23,11 @@ ARG celeborn_gid=10006
ENV CELEBORN_HOME=/opt/celeborn
ENV PATH=/opt/java/openjdk/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/opt/busybox:${CELEBORN_HOME}/sbin:${CELEBORN_HOME}/bin
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
RUN set -ex && \
apt-get update && \
- apt-get install -y bash tini busybox bind9-utils telnet net-tools procps krb5-user dnsutils && \
+ apt-get install -y bash tini busybox bind9-utils telnet net-tools procps krb5-user dnsutils gpg libsnappy1v5 gettext-base libjemalloc-dev && \
Review Comment:
seems the runtime library `libjemalloc2` is enough, as we don't need dev stuff
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 closed pull request #1824: [CELEBORN-900] Prefer to use jemalloc for memory allocation
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 closed pull request #1824: [CELEBORN-900] Prefer to use jemalloc for memory allocation
URL: https://github.com/apache/incubator-celeborn/pull/1824
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] cxzl25 commented on a diff in pull request #1824: [CELEBORN-900] Prefer to use jemalloc for memory allocation
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1319909938
##########
sbin/load-celeborn-env.sh:
##########
@@ -69,3 +69,24 @@ if [ "$CELEBORN_PID_DIR" = "" ]; then
export CELEBORN_PID_DIR="${CELEBORN_HOME}/pids"
fi
+# jemalloc memory allocator is enabled by default, you may set DISABLE_JEMALLOC to true in celeborn-env.sh if it's not as you wish.
Review Comment:
Not every OS has jemalloc installed, and now the start and stop commands will have multiple WARNING.
We can disable it by default, then enable it in `celeborn-env.sh`, and add instructions to the website or README.
```bash
./sbin/start-master.sh
```
```
WARNING: attempted to load jemalloc from /usr/lib/arm64-linux-gnu/libjemalloc.so and /usr/lib/x86_64-linux-gnu/libjemalloc.so but the library couldn't be found. glibc will be used instead.
WARNING: attempted to load jemalloc from /usr/lib/arm64-linux-gnu/libjemalloc.so and /usr/lib/x86_64-linux-gnu/libjemalloc.so but the library couldn't be found. glibc will be used instead.
starting org.apache.celeborn.service.deploy.master.Master
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1301244577
##########
docker/Dockerfile:
##########
@@ -35,6 +35,8 @@ RUN set -ex && \
useradd --uid=${celeborn_uid} --gid=${celeborn_gid} celeborn -d /home/celeborn -m && \
mkdir -p ${CELEBORN_HOME}
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
Review Comment:
`uname -m` would be a good idea, but does it work in ENV command?
Also, is it only beneficial inside the container? if not, we'd better supply a general solution. e.g. port the jemalloc detection and enabling logic into the bootstrap shell script, and introduce an env var like CELEBORN_DISABLE_JEMALLOC to make it configurable.
Then, the behavior is consistent inside and outside of the container:
- enable jemalloc by default when libjemalloc.so is available
- user can set CELEBORN_DISABLE_JEMALLOC=1 to explicitly disable the jemalloc usage
thought?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1301279152
##########
docker/Dockerfile:
##########
@@ -35,6 +35,8 @@ RUN set -ex && \
useradd --uid=${celeborn_uid} --gid=${celeborn_gid} celeborn -d /home/celeborn -m && \
mkdir -p ${CELEBORN_HOME}
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
Review Comment:
To be clear, what I mean "bootstrap shell script" is `sbin/load-celeborn-env.sh`(or maybe another one), not `docker-entrypoint.sh`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1301316397
##########
docker/Dockerfile:
##########
@@ -35,6 +35,8 @@ RUN set -ex && \
useradd --uid=${celeborn_uid} --gid=${celeborn_gid} celeborn -d /home/celeborn -m && \
mkdir -p ${CELEBORN_HOME}
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
Review Comment:
We can simply consider the Ubuntu/CentOS in the detection logic, and allow the user to overwrite the `LD_PRELOAD` in `conf/celeborn-env.sh` in an arbitrary location.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#issuecomment-1689259777
maybe "[CELEBORN-900] Prefer to use jemalloc for memory allocation"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Prefer to use jemalloc for memory allocation
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1319726021
##########
sbin/load-celeborn-env.sh:
##########
@@ -69,3 +69,24 @@ if [ "$CELEBORN_PID_DIR" = "" ]; then
export CELEBORN_PID_DIR="${CELEBORN_HOME}/pids"
fi
+# jemalloc memory allocator is enabled by default, you may set DISABLE_JEMALLOC to true in celeborn-env.sh if it's not as you wish.
+maybe_enable_jemalloc() {
+ if [ "${DISABLE_JEMALLOC:-false}" == "false" ]; then
+ JEMALLOC_PATH="/usr/lib/$(uname -m)-linux-gnu/libjemalloc.so"
+ JEMALLOC_FALLBACK="/usr/lib/x86_64-linux-gnu/libjemalloc.so"
+ if [ -f "$JEMALLOC_PATH" ]; then
+ export LD_PRELOAD=$LD_PRELOAD:$JEMALLOC_PATH
+ elif [ -f "$JEMALLOC_FALLBACK" ]; then
+ export LD_PRELOAD=$LD_PRELOAD:$JEMALLOC_FALLBACK
Review Comment:
```suggestion
if [ -f "$JEMALLOC_PATH" ]; then
export LD_PRELOAD="$LD_PRELOAD:$JEMALLOC_PATH"
elif [ -f "$JEMALLOC_FALLBACK" ]; then
export LD_PRELOAD="$LD_PRELOAD:$JEMALLOC_FALLBACK"
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] waitinfuture commented on pull request #1824: [CELEBORN-900] introduce jemalloc when deploying for flink on k8s
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#issuecomment-1687573237
cc @zwangsheng @pan3793 @RexXiong could you take a look at this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1301396484
##########
docker/Dockerfile:
##########
@@ -23,10 +23,11 @@ ARG celeborn_gid=10006
ENV CELEBORN_HOME=/opt/celeborn
ENV PATH=/opt/java/openjdk/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/opt/busybox:${CELEBORN_HOME}/sbin:${CELEBORN_HOME}/bin
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
RUN set -ex && \
apt-get update && \
- apt-get install -y bash tini busybox bind9-utils telnet net-tools procps krb5-user dnsutils && \
+ apt-get install -y bash tini busybox bind9-utils telnet net-tools procps krb5-user dnsutils gpg libsnappy1v5 gettext-base libjemalloc-dev && \
Review Comment:
Thanks for information, let's keep `libjemalloc-dev` then
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1301279152
##########
docker/Dockerfile:
##########
@@ -35,6 +35,8 @@ RUN set -ex && \
useradd --uid=${celeborn_uid} --gid=${celeborn_gid} celeborn -d /home/celeborn -m && \
mkdir -p ${CELEBORN_HOME}
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
Review Comment:
To be clear, what I mean "bootstrap shell script" is `sbin/load-celeborn-env.sh`(or maybe anther one), not `docker-entrypoint.sh`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] mddxhj commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "mddxhj (via GitHub)" <gi...@apache.org>.
mddxhj commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1301190062
##########
docker/Dockerfile:
##########
@@ -23,10 +23,11 @@ ARG celeborn_gid=10006
ENV CELEBORN_HOME=/opt/celeborn
ENV PATH=/opt/java/openjdk/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/opt/busybox:${CELEBORN_HOME}/sbin:${CELEBORN_HOME}/bin
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
RUN set -ex && \
apt-get update && \
- apt-get install -y bash tini busybox bind9-utils telnet net-tools procps krb5-user dnsutils && \
+ apt-get install -y bash tini busybox bind9-utils telnet net-tools procps krb5-user dnsutils gpg libsnappy1v5 gettext-base libjemalloc-dev && \
Review Comment:
@pan3793 Sorry for this, there's no need for introducing these three libs, I've fixed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1301322016
##########
docker/Dockerfile:
##########
@@ -35,6 +35,8 @@ RUN set -ex && \
useradd --uid=${celeborn_uid} --gid=${celeborn_gid} celeborn -d /home/celeborn -m && \
mkdir -p ${CELEBORN_HOME}
+ENV LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so
Review Comment:
@mddxhj could you make such changes if you don't have further concerns?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] mddxhj commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc for celeborn
Posted by "mddxhj (via GitHub)" <gi...@apache.org>.
mddxhj commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1319555107
##########
docker/Dockerfile:
##########
@@ -26,15 +26,14 @@ ENV PATH=/opt/java/openjdk/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin
RUN set -ex && \
apt-get update && \
- apt-get install -y bash tini busybox bind9-utils telnet net-tools procps krb5-user dnsutils && \
+ apt-get install -y bash tini busybox bind9-utils telnet net-tools procps krb5-user dnsutils libjemalloc-dev && \
ln -snf /bin/bash /bin/sh && \
rm -rf /var/cache/apt/* && \
mkdir /opt/busybox && \
busybox --install /opt/busybox && \
groupadd --gid=${celeborn_gid} celeborn && \
useradd --uid=${celeborn_uid} --gid=${celeborn_gid} celeborn -d /home/celeborn -m && \
mkdir -p ${CELEBORN_HOME}
-
Review Comment:
have fix it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Introduce jemalloc when deploying for flink on k8s
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1319485376
##########
docker/Dockerfile:
##########
@@ -26,15 +26,14 @@ ENV PATH=/opt/java/openjdk/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin
RUN set -ex && \
apt-get update && \
- apt-get install -y bash tini busybox bind9-utils telnet net-tools procps krb5-user dnsutils && \
+ apt-get install -y bash tini busybox bind9-utils telnet net-tools procps krb5-user dnsutils libjemalloc-dev && \
ln -snf /bin/bash /bin/sh && \
rm -rf /var/cache/apt/* && \
mkdir /opt/busybox && \
busybox --install /opt/busybox && \
groupadd --gid=${celeborn_gid} celeborn && \
useradd --uid=${celeborn_uid} --gid=${celeborn_gid} celeborn -d /home/celeborn -m && \
mkdir -p ${CELEBORN_HOME}
-
Review Comment:
nit: unnecessary change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Prefer to use jemalloc for memory allocation
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1319722284
##########
sbin/load-celeborn-env.sh:
##########
@@ -69,3 +69,24 @@ if [ "$CELEBORN_PID_DIR" = "" ]; then
export CELEBORN_PID_DIR="${CELEBORN_HOME}/pids"
fi
+# jemalloc memory allocator is enabled by default, you may set DISABLE_JEMALLOC to true in celeborn-env.sh if it's not as you wish.
+maybe_enable_jemalloc() {
+ if [ "${DISABLE_JEMALLOC:-false}" == "false" ]; then
+ JEMALLOC_PATH="/usr/lib/$(uname -m)-linux-gnu/libjemalloc.so"
+ JEMALLOC_FALLBACK="/usr/lib/x86_64-linux-gnu/libjemalloc.so"
+ if [ -f "$JEMALLOC_PATH" ]; then
+ export LD_PRELOAD=$LD_PRELOAD:$JEMALLOC_PATH
+ elif [ -f "$JEMALLOC_FALLBACK" ]; then
+ export LD_PRELOAD=$LD_PRELOAD:$JEMALLOC_FALLBACK
+ else
+ if [ "$JEMALLOC_PATH" = "$JEMALLOC_FALLBACK" ]; then
Review Comment:
```suggestion
if [ "$JEMALLOC_PATH" == "$JEMALLOC_FALLBACK" ]; then
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1824: [CELEBORN-900] Prefer to use jemalloc for memory allocation
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1824:
URL: https://github.com/apache/incubator-celeborn/pull/1824#discussion_r1319718172
##########
conf/celeborn-env.sh.template:
##########
@@ -31,4 +31,5 @@
# CELEBORN_WORKER_JAVA_OPTS="-XX:-PrintGC -XX:+PrintGCDetails -XX:+PrintGCTimeStamps -XX:+PrintGCDateStamps -Xloggc:gc-worker.out -Dio.netty.leakDetectionLevel=advanced"
# CELEBORN_MASTER_JAVA_OPTS="-XX:-PrintGC -XX:+PrintGCDetails -XX:+PrintGCTimeStamps -XX:+PrintGCDateStamps -Xloggc:gc-master.out -Dio.netty.leakDetectionLevel=advanced"
# CELEBORN_PID_DIR="$CELEBORN_HOME/pids"
-# CELEBORN_LOG_DIR="$CELEBORN_HOME/logs"
\ No newline at end of file
+# CELEBORN_LOG_DIR="$CELEBORN_HOME/logs"
+# DISABLE_JEMALLOC="false"
Review Comment:
```suggestion
# DISABLE_JEMALLOC="false"
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org