You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2021/01/13 10:34:12 UTC
[GitHub] [flink-docker] zentol opened a new pull request #54: [FLINK-20915][docker] Move docker entrypoint to distribution
zentol opened a new pull request #54:
URL: https://github.com/apache/flink-docker/pull/54
Counter-part to https://github.com/apache/flink/pull/14630.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-docker] rmetzger commented on a change in pull request #54: [FLINK-20915][docker] Move docker entrypoint to distribution
Posted by GitBox <gi...@apache.org>.
rmetzger commented on a change in pull request #54:
URL: https://github.com/apache/flink-docker/pull/54#discussion_r556621609
##########
File path: docker-entrypoint.sh
##########
@@ -41,60 +31,6 @@ drop_privs_cmd() {
fi
}
-copy_plugins_if_required() {
- if [ -z "$ENABLE_BUILT_IN_PLUGINS" ]; then
- return 0
- fi
-
- echo "Enabling required built-in plugins"
- for target_plugin in $(echo "$ENABLE_BUILT_IN_PLUGINS" | tr ';' ' '); do
- echo "Linking ${target_plugin} to plugin directory"
- plugin_name=${target_plugin%.jar}
-
- mkdir -p "${FLINK_HOME}/plugins/${plugin_name}"
- if [ ! -e "${FLINK_HOME}/opt/${target_plugin}" ]; then
- echo "Plugin ${target_plugin} does not exist. Exiting."
- exit 1
- else
- ln -fs "${FLINK_HOME}/opt/${target_plugin}" "${FLINK_HOME}/plugins/${plugin_name}"
- echo "Successfully enabled ${target_plugin}"
- fi
- done
-}
-
-set_config_option() {
- local option=$1
- local value=$2
-
- # escape periods for usage in regular expressions
- local escaped_option=$(echo ${option} | sed -e "s/\./\\\./g")
-
- # either override an existing entry, or append a new one
- if grep -E "^${escaped_option}:.*" "${CONF_FILE}" > /dev/null; then
- sed -i -e "s/${escaped_option}:.*/$option: $value/g" "${CONF_FILE}"
- else
- echo "${option}: ${value}" >> "${CONF_FILE}"
- fi
-}
-
-set_common_options() {
- set_config_option jobmanager.rpc.address ${JOB_MANAGER_RPC_ADDRESS}
- set_config_option blob.server.port 6124
- set_config_option query.server.port 6125
-}
-
-prepare_job_manager_start() {
- echo "Starting Job Manager"
- copy_plugins_if_required
-
- set_common_options
-
- if [ -n "${FLINK_PROPERTIES}" ]; then
- echo "${FLINK_PROPERTIES}" >> "${CONF_FILE}"
- fi
- envsubst < "${CONF_FILE}" > "${CONF_FILE}.tmp" && mv "${CONF_FILE}.tmp" "${CONF_FILE}"
-}
-
disable_jemalloc_env() {
# use nameref '_args' to update the passed 'args' within function
local -n _args=$1
Review comment:
COMMAND_DISABLE_JEMALLOC is not defined in this file (anymore)
##########
File path: docker-entrypoint.sh
##########
@@ -107,80 +43,10 @@ disable_jemalloc_env() {
}
args=("$@")
-if [ "$1" = "help" ]; then
- printf "Usage: $(basename "$0") (jobmanager|${COMMAND_STANDALONE}|taskmanager|${COMMAND_HISTORY_SERVER}) [${COMMAND_DISABLE_JEMALLOC}]\n"
- printf " Or $(basename "$0") help\n\n"
- printf "By default, Flink image adopts jemalloc as default memory allocator and will disable jemalloc if option '${COMMAND_DISABLE_JEMALLOC}' given.\n"
- exit 0
-elif [ "$1" = "jobmanager" ]; then
- args=("${args[@]:1}")
- disable_jemalloc_env args
-
- prepare_job_manager_start
-
- exec $(drop_privs_cmd) "$FLINK_HOME/bin/jobmanager.sh" start-foreground "${args[@]}"
-elif [ "$1" = ${COMMAND_STANDALONE} ]; then
- args=("${args[@]:1}")
- disable_jemalloc_env args
-
- prepare_job_manager_start
-
- exec $(drop_privs_cmd) "$FLINK_HOME/bin/standalone-job.sh" start-foreground "${args[@]}"
-elif [ "$1" = ${COMMAND_HISTORY_SERVER} ]; then
- args=("${args[@]:1}")
- disable_jemalloc_env args
-
- echo "Starting History Server"
- copy_plugins_if_required
-
- if [ -n "${FLINK_PROPERTIES}" ]; then
- echo "${FLINK_PROPERTIES}" >> "${CONF_FILE}"
- fi
- envsubst < "${CONF_FILE}" > "${CONF_FILE}.tmp" && mv "${CONF_FILE}.tmp" "${CONF_FILE}"
-
- exec $(drop_privs_cmd) "$FLINK_HOME/bin/historyserver.sh" start-foreground "${args[@]}"
-elif [ "$1" = "taskmanager" ]; then
- args=("${args[@]:1}")
- disable_jemalloc_env args
-
- echo "Starting Task Manager"
- copy_plugins_if_required
-
- TASK_MANAGER_NUMBER_OF_TASK_SLOTS=${TASK_MANAGER_NUMBER_OF_TASK_SLOTS:-$(grep -c ^processor /proc/cpuinfo)}
-
- set_common_options
- set_config_option taskmanager.numberOfTaskSlots ${TASK_MANAGER_NUMBER_OF_TASK_SLOTS}
-
- if [ -n "${FLINK_PROPERTIES}" ]; then
- echo "${FLINK_PROPERTIES}" >> "${CONF_FILE}"
- fi
- envsubst < "${CONF_FILE}" > "${CONF_FILE}.tmp" && mv "${CONF_FILE}.tmp" "${CONF_FILE}"
-
- exec $(drop_privs_cmd) "$FLINK_HOME/bin/taskmanager.sh" start-foreground "${args[@]}"
-elif [ "$1" = "$COMMAND_NATIVE_KUBERNETES" ]; then
- args=("${args[@]:1}")
- disable_jemalloc_env args
-
- copy_plugins_if_required
-
- export _FLINK_HOME_DETERMINED=true
- . $FLINK_HOME/bin/config.sh
- export FLINK_CLASSPATH="`constructFlinkClassPath`:$INTERNAL_HADOOP_CLASSPATHS"
- # Start commands for jobmanager and taskmanager are generated by Flink internally.
- echo "Start command: ${args[@]}"
- exec $(drop_privs_cmd) bash -c "${args[@]}"
-fi
args=("${args[@]}")
disable_jemalloc_env args
-copy_plugins_if_required
-
-# Set the Flink related environments
-export _FLINK_HOME_DETERMINED=true
-. $FLINK_HOME/bin/config.sh
-export FLINK_CLASSPATH="`constructFlinkClassPath`:$INTERNAL_HADOOP_CLASSPATHS"
-
# Running command in pass-through mode
Review comment:
```suggestion
# Running command in pass-through mode: The actual docker-entrypoint.sh logic is in the Flink distribution.
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-docker] rmetzger commented on a change in pull request #54: [FLINK-20915][docker] Move docker entrypoint to distribution
Posted by GitBox <gi...@apache.org>.
rmetzger commented on a change in pull request #54:
URL: https://github.com/apache/flink-docker/pull/54#discussion_r556641582
##########
File path: docker-entrypoint.sh
##########
@@ -41,60 +33,6 @@ drop_privs_cmd() {
fi
}
-copy_plugins_if_required() {
- if [ -z "$ENABLE_BUILT_IN_PLUGINS" ]; then
- return 0
- fi
-
- echo "Enabling required built-in plugins"
- for target_plugin in $(echo "$ENABLE_BUILT_IN_PLUGINS" | tr ';' ' '); do
- echo "Linking ${target_plugin} to plugin directory"
- plugin_name=${target_plugin%.jar}
-
- mkdir -p "${FLINK_HOME}/plugins/${plugin_name}"
- if [ ! -e "${FLINK_HOME}/opt/${target_plugin}" ]; then
- echo "Plugin ${target_plugin} does not exist. Exiting."
- exit 1
- else
- ln -fs "${FLINK_HOME}/opt/${target_plugin}" "${FLINK_HOME}/plugins/${plugin_name}"
- echo "Successfully enabled ${target_plugin}"
- fi
- done
-}
-
-set_config_option() {
- local option=$1
- local value=$2
-
- # escape periods for usage in regular expressions
- local escaped_option=$(echo ${option} | sed -e "s/\./\\\./g")
-
- # either override an existing entry, or append a new one
- if grep -E "^${escaped_option}:.*" "${CONF_FILE}" > /dev/null; then
- sed -i -e "s/${escaped_option}:.*/$option: $value/g" "${CONF_FILE}"
- else
- echo "${option}: ${value}" >> "${CONF_FILE}"
- fi
-}
-
-set_common_options() {
- set_config_option jobmanager.rpc.address ${JOB_MANAGER_RPC_ADDRESS}
- set_config_option blob.server.port 6124
- set_config_option query.server.port 6125
-}
-
-prepare_job_manager_start() {
- echo "Starting Job Manager"
- copy_plugins_if_required
-
- set_common_options
-
- if [ -n "${FLINK_PROPERTIES}" ]; then
- echo "${FLINK_PROPERTIES}" >> "${CONF_FILE}"
- fi
- envsubst < "${CONF_FILE}" > "${CONF_FILE}.tmp" && mv "${CONF_FILE}.tmp" "${CONF_FILE}"
-}
-
Review comment:
@wangyang0918 mentioned that he'd prefer controlling jemalloc via an environment variable
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-docker] zentol closed pull request #54: [FLINK-20915][docker] Move docker entrypoint to distribution
Posted by GitBox <gi...@apache.org>.
zentol closed pull request #54:
URL: https://github.com/apache/flink-docker/pull/54
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-docker] rmetzger commented on a change in pull request #54: [FLINK-20915][docker] Move docker entrypoint to distribution
Posted by GitBox <gi...@apache.org>.
rmetzger commented on a change in pull request #54:
URL: https://github.com/apache/flink-docker/pull/54#discussion_r557342677
##########
File path: docker-entrypoint.sh
##########
@@ -41,60 +33,6 @@ drop_privs_cmd() {
fi
}
-copy_plugins_if_required() {
- if [ -z "$ENABLE_BUILT_IN_PLUGINS" ]; then
- return 0
- fi
-
- echo "Enabling required built-in plugins"
- for target_plugin in $(echo "$ENABLE_BUILT_IN_PLUGINS" | tr ';' ' '); do
- echo "Linking ${target_plugin} to plugin directory"
- plugin_name=${target_plugin%.jar}
-
- mkdir -p "${FLINK_HOME}/plugins/${plugin_name}"
- if [ ! -e "${FLINK_HOME}/opt/${target_plugin}" ]; then
- echo "Plugin ${target_plugin} does not exist. Exiting."
- exit 1
- else
- ln -fs "${FLINK_HOME}/opt/${target_plugin}" "${FLINK_HOME}/plugins/${plugin_name}"
- echo "Successfully enabled ${target_plugin}"
- fi
- done
-}
-
-set_config_option() {
- local option=$1
- local value=$2
-
- # escape periods for usage in regular expressions
- local escaped_option=$(echo ${option} | sed -e "s/\./\\\./g")
-
- # either override an existing entry, or append a new one
- if grep -E "^${escaped_option}:.*" "${CONF_FILE}" > /dev/null; then
- sed -i -e "s/${escaped_option}:.*/$option: $value/g" "${CONF_FILE}"
- else
- echo "${option}: ${value}" >> "${CONF_FILE}"
- fi
-}
-
-set_common_options() {
- set_config_option jobmanager.rpc.address ${JOB_MANAGER_RPC_ADDRESS}
- set_config_option blob.server.port 6124
- set_config_option query.server.port 6125
-}
-
-prepare_job_manager_start() {
- echo "Starting Job Manager"
- copy_plugins_if_required
-
- set_common_options
-
- if [ -n "${FLINK_PROPERTIES}" ]; then
- echo "${FLINK_PROPERTIES}" >> "${CONF_FILE}"
- fi
- envsubst < "${CONF_FILE}" > "${CONF_FILE}.tmp" && mv "${CONF_FILE}.tmp" "${CONF_FILE}"
-}
-
Review comment:
Okay, let's do it later 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-docker] zentol commented on a change in pull request #54: [FLINK-20915][docker] Move docker entrypoint to distribution
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #54:
URL: https://github.com/apache/flink-docker/pull/54#discussion_r556653073
##########
File path: docker-entrypoint.sh
##########
@@ -41,60 +33,6 @@ drop_privs_cmd() {
fi
}
-copy_plugins_if_required() {
- if [ -z "$ENABLE_BUILT_IN_PLUGINS" ]; then
- return 0
- fi
-
- echo "Enabling required built-in plugins"
- for target_plugin in $(echo "$ENABLE_BUILT_IN_PLUGINS" | tr ';' ' '); do
- echo "Linking ${target_plugin} to plugin directory"
- plugin_name=${target_plugin%.jar}
-
- mkdir -p "${FLINK_HOME}/plugins/${plugin_name}"
- if [ ! -e "${FLINK_HOME}/opt/${target_plugin}" ]; then
- echo "Plugin ${target_plugin} does not exist. Exiting."
- exit 1
- else
- ln -fs "${FLINK_HOME}/opt/${target_plugin}" "${FLINK_HOME}/plugins/${plugin_name}"
- echo "Successfully enabled ${target_plugin}"
- fi
- done
-}
-
-set_config_option() {
- local option=$1
- local value=$2
-
- # escape periods for usage in regular expressions
- local escaped_option=$(echo ${option} | sed -e "s/\./\\\./g")
-
- # either override an existing entry, or append a new one
- if grep -E "^${escaped_option}:.*" "${CONF_FILE}" > /dev/null; then
- sed -i -e "s/${escaped_option}:.*/$option: $value/g" "${CONF_FILE}"
- else
- echo "${option}: ${value}" >> "${CONF_FILE}"
- fi
-}
-
-set_common_options() {
- set_config_option jobmanager.rpc.address ${JOB_MANAGER_RPC_ADDRESS}
- set_config_option blob.server.port 6124
- set_config_option query.server.port 6125
-}
-
-prepare_job_manager_start() {
- echo "Starting Job Manager"
- copy_plugins_if_required
-
- set_common_options
-
- if [ -n "${FLINK_PROPERTIES}" ]; then
- echo "${FLINK_PROPERTIES}" >> "${CONF_FILE}"
- fi
- envsubst < "${CONF_FILE}" > "${CONF_FILE}.tmp" && mv "${CONF_FILE}.tmp" "${CONF_FILE}"
-}
-
Review comment:
Out-of-scope of this PR as far as I'm concerned. That's an API change that I would move to a follow-up.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-docker] zentol closed pull request #54: [FLINK-20915][docker] Move docker entrypoint to distribution
Posted by GitBox <gi...@apache.org>.
zentol closed pull request #54:
URL: https://github.com/apache/flink-docker/pull/54
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-docker] zentol commented on pull request #54: [FLINK-20915][docker] Move docker entrypoint to distribution
Posted by GitBox <gi...@apache.org>.
zentol commented on pull request #54:
URL: https://github.com/apache/flink-docker/pull/54#issuecomment-760191543
I have updated the PR. The previous version broke the jemalloc switch since we were checking the wrong argument index. I did not want to deal with mutating arrays, and instead refactored the switch to a environment variable and added tests for the behavior.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-docker] wangyang0918 commented on a change in pull request #54: [FLINK-20915][docker] Move docker entrypoint to distribution
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on a change in pull request #54:
URL: https://github.com/apache/flink-docker/pull/54#discussion_r557077935
##########
File path: docker-entrypoint.sh
##########
@@ -41,60 +33,6 @@ drop_privs_cmd() {
fi
}
-copy_plugins_if_required() {
- if [ -z "$ENABLE_BUILT_IN_PLUGINS" ]; then
- return 0
- fi
-
- echo "Enabling required built-in plugins"
- for target_plugin in $(echo "$ENABLE_BUILT_IN_PLUGINS" | tr ';' ' '); do
- echo "Linking ${target_plugin} to plugin directory"
- plugin_name=${target_plugin%.jar}
-
- mkdir -p "${FLINK_HOME}/plugins/${plugin_name}"
- if [ ! -e "${FLINK_HOME}/opt/${target_plugin}" ]; then
- echo "Plugin ${target_plugin} does not exist. Exiting."
- exit 1
- else
- ln -fs "${FLINK_HOME}/opt/${target_plugin}" "${FLINK_HOME}/plugins/${plugin_name}"
- echo "Successfully enabled ${target_plugin}"
- fi
- done
-}
-
-set_config_option() {
- local option=$1
- local value=$2
-
- # escape periods for usage in regular expressions
- local escaped_option=$(echo ${option} | sed -e "s/\./\\\./g")
-
- # either override an existing entry, or append a new one
- if grep -E "^${escaped_option}:.*" "${CONF_FILE}" > /dev/null; then
- sed -i -e "s/${escaped_option}:.*/$option: $value/g" "${CONF_FILE}"
- else
- echo "${option}: ${value}" >> "${CONF_FILE}"
- fi
-}
-
-set_common_options() {
- set_config_option jobmanager.rpc.address ${JOB_MANAGER_RPC_ADDRESS}
- set_config_option blob.server.port 6124
- set_config_option query.server.port 6125
-}
-
-prepare_job_manager_start() {
- echo "Starting Job Manager"
- copy_plugins_if_required
-
- set_common_options
-
- if [ -n "${FLINK_PROPERTIES}" ]; then
- echo "${FLINK_PROPERTIES}" >> "${CONF_FILE}"
- fi
- envsubst < "${CONF_FILE}" > "${CONF_FILE}.tmp" && mv "${CONF_FILE}.tmp" "${CONF_FILE}"
-}
-
Review comment:
I believe setting the env(`export DISABLE_JEMALLOC=true`) is more convenient to use and could be easily integrated in all modes(docker, standalone, native-k8s). I agree that this could be done as a follow-up.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org