zentol commented on a change in pull request #11839:
URL: https://github.com/apache/flink/pull/11839#discussion_r425726762
##########
File path: flink-dist/src/main/flink-bin/bin/flink-console.sh
##########
@@ -70,4 +97,10 @@ if [[ ${JAVA_VERSION} =~ ${IS_NUMBER} ]]; then
fi
echo "Starting $SERVICE as a console application on host $HOSTNAME."
+
+echo $$ >> "$pid" 2>/dev/null
Review comment:
so `$$` returns the PID of the current process?
> and then replace the current program with specified SERVICE in the same
process
With this you mean the exec call? if so, I'd omit it, because it raises more
questions than it answers. (most notably due to describing behavior several
lines down)
##########
File path: flink-dist/src/main/flink-bin/bin/flink-console.sh
##########
@@ -58,7 +58,34 @@ esac
FLINK_TM_CLASSPATH=`constructFlinkClassPath`
-log_setting=("-Dlog4j.configuration=file:${FLINK_CONF_DIR}/log4j-console.properties"
"-Dlog4j.configurationFile=file:${FLINK_CONF_DIR}/log4j-console.properties"
"-Dlogback.configurationFile=file:${FLINK_CONF_DIR}/logback-console.xml")
+if [ "$FLINK_IDENT_STRING" = "" ]; then
+ FLINK_IDENT_STRING="$USER"
+fi
+
+pid=$FLINK_PID_DIR/flink-$FLINK_IDENT_STRING-$SERVICE.pid
+mkdir -p "$FLINK_PID_DIR"
+# The lock needs to be released after use because this script is started
foreground
+command -v flock >/dev/null 2>&1
+flock_exist=$?
+if [[ ${flock_exist} -eq 0 ]]; then
+ exec 200<"$FLINK_PID_DIR"
+ flock 200
+fi
+# Remove the pid file when all the processes are dead
+if [ -f "$pid" ];then
+ all_dead=0
+ while read each_pid; do
+ kill -0 $each_pid > /dev/null 2>&1
Review comment:
add a comment that this checks whether the process is still running
##########
File path: flink-dist/src/main/flink-bin/bin/flink-console.sh
##########
@@ -58,7 +58,34 @@ esac
FLINK_TM_CLASSPATH=`constructFlinkClassPath`
-log_setting=("-Dlog4j.configuration=file:${FLINK_CONF_DIR}/log4j-console.properties"
"-Dlog4j.configurationFile=file:${FLINK_CONF_DIR}/log4j-console.properties"
"-Dlogback.configurationFile=file:${FLINK_CONF_DIR}/logback-console.xml")
+if [ "$FLINK_IDENT_STRING" = "" ]; then
+ FLINK_IDENT_STRING="$USER"
+fi
+
+pid=$FLINK_PID_DIR/flink-$FLINK_IDENT_STRING-$SERVICE.pid
+mkdir -p "$FLINK_PID_DIR"
+# The lock needs to be released after use because this script is started
foreground
+command -v flock >/dev/null 2>&1
+flock_exist=$?
+if [[ ${flock_exist} -eq 0 ]]; then
+ exec 200<"$FLINK_PID_DIR"
+ flock 200
+fi
+# Remove the pid file when all the processes are dead
+if [ -f "$pid" ];then
+ all_dead=0
Review comment:
Shouldn't this cleanup be done in a trap of some sort? Like on-exit
remove the PID of this process from the file, and then _maybe_ delete it if
empty?
##########
File path: flink-dist/src/main/flink-bin/bin/flink-console.sh
##########
@@ -70,4 +97,10 @@ if [[ ${JAVA_VERSION} =~ ${IS_NUMBER} ]]; then
fi
echo "Starting $SERVICE as a console application on host $HOSTNAME."
+
+echo $$ >> "$pid" 2>/dev/null
+
+# Release the lock
Review comment:
```suggestion
# Release the lock, because the java process runs in the foreground and
would block other processes from modifying the pid file
```
----------------------------------------------------------------
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:
[email protected]
