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 2020/04/22 09:44:51 UTC
[GitHub] [flink-docker] azagrebin commented on a change in pull request #12: [FLINK-17187] Add utility method for setting config options
azagrebin commented on a change in pull request #12:
URL: https://github.com/apache/flink-docker/pull/12#discussion_r412818958
##########
File path: docker-entrypoint.sh
##########
@@ -56,6 +56,21 @@ copy_plugins_if_required() {
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")
Review comment:
```suggestion
local escaped_option=${option//\./\\\.}
```
##########
File path: docker-entrypoint.sh
##########
@@ -56,6 +56,21 @@ copy_plugins_if_required() {
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
Review comment:
```suggestion
if grep -E "^${escaped_option}:.*" "${CONF_FILE}" > /dev/null; then
```
##########
File path: docker-entrypoint.sh
##########
@@ -56,6 +56,21 @@ copy_plugins_if_required() {
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}"
Review comment:
Just some side thoughts:
I suppose the purpose of this is to rewrite the value in the default `flink-conf.yaml` shipped with Flink release.
If we decide to expose an env var to point to custom config location (e.g. a mounted volume)
then we have to reconsider this step or do it before to make sure we rewrite the default `flink-conf.yaml`.
##########
File path: docker-entrypoint.sh
##########
@@ -64,23 +79,9 @@ elif [ "$1" = "jobmanager" ]; then
echo "Starting Job Manager"
copy_plugins_if_required
- if grep -E "^jobmanager\.rpc\.address:.*" "${CONF_FILE}" > /dev/null; then
- sed -i -e "s/jobmanager\.rpc\.address:.*/jobmanager.rpc.address: ${JOB_MANAGER_RPC_ADDRESS}/g" "${CONF_FILE}"
- else
- echo "jobmanager.rpc.address: ${JOB_MANAGER_RPC_ADDRESS}" >> "${CONF_FILE}"
- fi
-
- if grep -E "^blob\.server\.port:.*" "${CONF_FILE}" > /dev/null; then
- sed -i -e "s/blob\.server\.port:.*/blob.server.port: 6124/g" "${CONF_FILE}"
- else
- echo "blob.server.port: 6124" >> "${CONF_FILE}"
- fi
-
- if grep -E "^query\.server\.port:.*" "${CONF_FILE}" > /dev/null; then
- sed -i -e "s/query\.server\.port:.*/query.server.port: 6125/g" "${CONF_FILE}"
- else
- echo "query.server.port: 6125" >> "${CONF_FILE}"
- fi
+ set_config_option jobmanager.rpc.address ${JOB_MANAGER_RPC_ADDRESS}
+ set_config_option blob.server.port 6124
+ set_config_option query.server.port 6125
Review comment:
do you want to deduplicate this further? e.g.:
```
copy_plugins_if_required
set_config_option jobmanager.rpc.address ${JOB_MANAGER_RPC_ADDRESS}
set_config_option blob.server.port 6124
set_config_option query.server.port 6125
FLINK_PROPERTIES ...
```
->
```
configure_jm() / common_config() / etc ..
```
or in a later PR?
##########
File path: docker-entrypoint.sh
##########
@@ -96,29 +97,10 @@ elif [ "$1" = "taskmanager" ]; then
TASK_MANAGER_NUMBER_OF_TASK_SLOTS=${TASK_MANAGER_NUMBER_OF_TASK_SLOTS:-$(grep -c ^processor /proc/cpuinfo)}
- if grep -E "^jobmanager\.rpc\.address:.*" "${CONF_FILE}" > /dev/null; then
- sed -i -e "s/jobmanager\.rpc\.address:.*/jobmanager.rpc.address: ${JOB_MANAGER_RPC_ADDRESS}/g" "${CONF_FILE}"
- else
- echo "jobmanager.rpc.address: ${JOB_MANAGER_RPC_ADDRESS}" >> "${CONF_FILE}"
- fi
-
- if grep -E "^taskmanager\.numberOfTaskSlots:.*" "${CONF_FILE}" > /dev/null; then
- sed -i -e "s/taskmanager\.numberOfTaskSlots:.*/taskmanager.numberOfTaskSlots: ${TASK_MANAGER_NUMBER_OF_TASK_SLOTS}/g" "${CONF_FILE}"
- else
- echo "taskmanager.numberOfTaskSlots: ${TASK_MANAGER_NUMBER_OF_TASK_SLOTS}" >> "${CONF_FILE}"
- fi
-
- if grep -E "^blob\.server\.port:.*" "${CONF_FILE}" > /dev/null; then
- sed -i -e "s/blob\.server\.port:.*/blob.server.port: 6124/g" "${CONF_FILE}"
- else
- echo "blob.server.port: 6124" >> "${CONF_FILE}"
- fi
-
- if grep -E "^query\.server\.port:.*" "${CONF_FILE}" > /dev/null; then
- sed -i -e "s/query\.server\.port:.*/query.server.port: 6125/g" "${CONF_FILE}"
- else
- echo "query.server.port: 6125" >> "${CONF_FILE}"
- fi
+ set_config_option jobmanager.rpc.address ${JOB_MANAGER_RPC_ADDRESS}
+ set_config_option taskmanager.numberOfTaskSlots ${TASK_MANAGER_NUMBER_OF_TASK_SLOTS}
+ set_config_option blob.server.port 6124
+ set_config_option query.server.port 6125
if [ -n "${FLINK_PROPERTIES}" ]; then
echo "${FLINK_PROPERTIES}" >> "${CONF_FILE}"
Review comment:
I guess there is no evil in duplicated config options perse.
Just, maybe, it would be cleaner to iterate over lines in `FLINK_PROPERTIES` and call `set_config_option`
btw, this could be also some `custom_config()` or so
----------------------------------------------------------------
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