[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r342641241
##
File path: flink-dist/src/main/flink-bin/bin/flink-console.sh
##
@@ -56,8 +56,23 @@ case $SERVICE in
;;
esac
+if [ "$FLINK_IDENT_STRING" = "" ]; then
+FLINK_IDENT_STRING="$USER"
+fi
+
FLINK_TM_CLASSPATH=`constructFlinkClassPath`
+FLINK_LOG_PREFIX="${FLINK_LOG_DIR}/flink-${FLINK_IDENT_STRING}-${SERVICE}"
+log="${FLINK_LOG_PREFIX}.log"
+out="${FLINK_LOG_PREFIX}.out"
Review comment:
I think we don't need these two variables.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r341645333
##
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java
##
@@ -513,6 +514,76 @@ public static Configuration
cloneConfiguration(Configuration configuration) {
return clonedConfiguration;
}
+ /**
+* Format the default gc logging options
+* @param logDirectory to save the gc log
+* @return the formatted gc logging options string
+*/
+ public static String getGCLoggingOpts(String logDirectory) {
+return "-Xloggc:" + logDirectory + "/gc.log " +
+ "-XX:+PrintGCApplicationStoppedTime " +
+ "-XX:+PrintGCDetails " +
+ "-XX:+PrintGCDateStamps " +
+ "-XX:+UseGCLogFileRotation " +
+ "-XX:NumberOfGCLogFiles=10 " +
+ "-XX:GCLogFileSize=10M " +
+ "-XX:+PrintPromotionFailure " +
+ "-XX:+PrintGCCause";
+ }
+
+ /**
+* Format the default heapdump options
+* @param appId application id
+* @param ident the ident of the process, taskmanager/jobmanager
+* @param logDirectory to print some logs
+* @param heapdumpDir to save heap dump file
+* @return the formatted heapdump options string
+*/
+ public static String getHeapdumpOpts(String appId, String ident, String
logDirectory, String heapdumpDir) {
+String dumpDestName = String.format("flink-%s-heapdump.hprof", ident);
+String dumpFileDestPath = new File(heapdumpDir, appId + "-" +
dumpDestName).getAbsolutePath();
+String oomScript = String.format("echo -e 'OutOfMemoryError! Killing
current process %%p...\n" +
+"Check gc logs and heapdump file(%s) for details.' > " +
+logDirectory + "/%s.err; " +
+"kill -9 %%p",
+ dumpFileDestPath, ident);
+return String.format("-XX:+HeapDumpOnOutOfMemoryError " +
+"-XX:HeapDumpPath=%s " +
+"-XX:OnOutOfMemoryError=\"%s\"",
+ dumpFileDestPath,
+ oomScript);
+ }
+
+ /**
+* Get the common jvm options
+* @param appId application id
+* @param ident the ident of the process, taskmanager/jobmanager
+* @param logDirectory to print some logs
+ * @param conf flink configuration
+ */
+ public static String getCommonJvmOpts(
+ String appId,
+ String ident,
+ String logDirectory,
+ Configuration conf) {
+String commonOpts = "";
+ boolean enableGCLogging =
conf.getBoolean(CoreOptions.FLINK_JVM_DEFAULT_GC_LOGGING);
+if (enableGCLogging) {
+ // Add default gc logging options if enabled
+ commonOpts += getGCLoggingOpts(logDirectory);
+}
+ boolean enableHeapDump =
conf.getBoolean(CoreOptions.FLINK_JVM_HEAPDUMP_ON_OOM);
+if (enableHeapDump) {
+ // Add default heap dump options if enabled
+ String heapdumpDir =
conf.getString(CoreOptions.FLINK_JVM_HEAPDUMP_DIRECTORY);
+ commonOpts += " " + getHeapdumpOpts(appId, ident, logDirectory,
heapdumpDir);
+}
+if (conf.getString(CoreOptions.FLINK_JVM_OPTIONS).length() > 0) {
+ commonOpts += " " +
conf.getString(CoreOptions.FLINK_JVM_OPTIONS);
+}
+return commonOpts;
+ }
Review comment:
Here as well. Please use tabs.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r341643976
##
File path: flink-dist/src/main/flink-bin/mesos-bin/mesos-appmaster-job.sh
##
@@ -31,6 +31,13 @@ CC_CLASSPATH=`manglePathList
$(constructFlinkClassPath):$INTERNAL_HADOOP_CLASSPA
log="${FLINK_LOG_DIR}/flink-${FLINK_IDENT_STRING}-mesos-appmaster-${HOSTNAME}.log"
log_setting="-Dlog.file="$log"
-Dlog4j.configuration=file:"$FLINK_CONF_DIR"/log4j.properties
-Dlogback.configurationFile=file:"$FLINK_CONF_DIR"/logback.xml"
+gclog="${FLINK_LOG_DIR}/flink-${FLINK_IDENT_STRING}-mesos-appmaster-${HOSTNAME}.gc_log"
+
+FLINK_HEAPDUMP_NAME="flink-${FLINK_IDENT_STRING}-mesos-appmaster-${HOSTNAME}.hprof"
+rm -rf ${FLINK_JVM_HEAPDUMP_DIRECTORY}/${FLINK_HEAPDUMP_NAME}
+FLINK_JVM_GC_LOGGING_OPTS=`getGCLoggingOpts $gclog`
+FLINK_JVM_HEAPDUMP_OPTS=`getHeapdumpOpts $FLINK_HEAPDUMP_NAME $log`
Review comment:
```suggestion
FLINK_JVM_HEAPDUMP_OPTS=$(getHeapdumpOpts $FLINK_HEAPDUMP_NAME ${log})
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r341644104
##
File path: flink-dist/src/main/flink-bin/mesos-bin/mesos-appmaster.sh
##
@@ -31,6 +31,13 @@ CC_CLASSPATH=`manglePathList
$(constructFlinkClassPath):$INTERNAL_HADOOP_CLASSPA
log="${FLINK_LOG_DIR}/flink-${FLINK_IDENT_STRING}-mesos-appmaster-${HOSTNAME}.log"
log_setting="-Dlog.file="$log"
-Dlog4j.configuration=file:"$FLINK_CONF_DIR"/log4j.properties
-Dlogback.configurationFile=file:"$FLINK_CONF_DIR"/logback.xml"
+gclog="${FLINK_LOG_DIR}/flink-${FLINK_IDENT_STRING}-mesos-appmaster-${HOSTNAME}.gc_log"
+
+FLINK_HEAPDUMP_NAME="flink-${FLINK_IDENT_STRING}-mesos-appmaster-${HOSTNAME}.hprof"
+rm -rf ${FLINK_JVM_HEAPDUMP_DIRECTORY}/${FLINK_HEAPDUMP_NAME}
+FLINK_JVM_GC_LOGGING_OPTS=`getGCLoggingOpts $gclog`
+FLINK_JVM_HEAPDUMP_OPTS=`getHeapdumpOpts $FLINK_HEAPDUMP_NAME $log`
Review comment:
```suggestion
FLINK_JVM_HEAPDUMP_OPTS=$(getHeapdumpOpts $FLINK_HEAPDUMP_NAME ${log})
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r341645497
##
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java
##
@@ -513,6 +514,76 @@ public static Configuration
cloneConfiguration(Configuration configuration) {
return clonedConfiguration;
}
+ /**
+* Format the default gc logging options
+* @param logDirectory to save the gc log
+* @return the formatted gc logging options string
+*/
+ public static String getGCLoggingOpts(String logDirectory) {
+return "-Xloggc:" + logDirectory + "/gc.log " +
+ "-XX:+PrintGCApplicationStoppedTime " +
+ "-XX:+PrintGCDetails " +
+ "-XX:+PrintGCDateStamps " +
+ "-XX:+UseGCLogFileRotation " +
+ "-XX:NumberOfGCLogFiles=10 " +
+ "-XX:GCLogFileSize=10M " +
+ "-XX:+PrintPromotionFailure " +
+ "-XX:+PrintGCCause";
+ }
+
+ /**
+* Format the default heapdump options
+* @param appId application id
+* @param ident the ident of the process, taskmanager/jobmanager
+* @param logDirectory to print some logs
+* @param heapdumpDir to save heap dump file
+* @return the formatted heapdump options string
+*/
+ public static String getHeapdumpOpts(String appId, String ident, String
logDirectory, String heapdumpDir) {
+String dumpDestName = String.format("flink-%s-heapdump.hprof", ident);
+String dumpFileDestPath = new File(heapdumpDir, appId + "-" +
dumpDestName).getAbsolutePath();
+String oomScript = String.format("echo -e 'OutOfMemoryError! Killing
current process %%p...\n" +
+"Check gc logs and heapdump file(%s) for details.' > " +
+logDirectory + "/%s.err; " +
+"kill -9 %%p",
+ dumpFileDestPath, ident);
+return String.format("-XX:+HeapDumpOnOutOfMemoryError " +
+"-XX:HeapDumpPath=%s " +
+"-XX:OnOutOfMemoryError=\"%s\"",
+ dumpFileDestPath,
+ oomScript);
+ }
+
+ /**
+* Get the common jvm options
+* @param appId application id
+* @param ident the ident of the process, taskmanager/jobmanager
+* @param logDirectory to print some logs
+ * @param conf flink configuration
+ */
+ public static String getCommonJvmOpts(
+ String appId,
+ String ident,
+ String logDirectory,
+ Configuration conf) {
+String commonOpts = "";
+ boolean enableGCLogging =
conf.getBoolean(CoreOptions.FLINK_JVM_DEFAULT_GC_LOGGING);
+if (enableGCLogging) {
+ // Add default gc logging options if enabled
+ commonOpts += getGCLoggingOpts(logDirectory);
+}
+ boolean enableHeapDump =
conf.getBoolean(CoreOptions.FLINK_JVM_HEAPDUMP_ON_OOM);
+if (enableHeapDump) {
+ // Add default heap dump options if enabled
+ String heapdumpDir =
conf.getString(CoreOptions.FLINK_JVM_HEAPDUMP_DIRECTORY);
+ commonOpts += " " + getHeapdumpOpts(appId, ident, logDirectory,
heapdumpDir);
+}
+if (conf.getString(CoreOptions.FLINK_JVM_OPTIONS).length() > 0) {
+ commonOpts += " " +
conf.getString(CoreOptions.FLINK_JVM_OPTIONS);
+}
+return commonOpts;
+ }
Review comment:
Also the formatting is off due to some lines being indented via tabs and
other via spaces.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r341645233
##
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java
##
@@ -513,6 +514,76 @@ public static Configuration
cloneConfiguration(Configuration configuration) {
return clonedConfiguration;
}
+ /**
+* Format the default gc logging options
+* @param logDirectory to save the gc log
+* @return the formatted gc logging options string
+*/
+ public static String getGCLoggingOpts(String logDirectory) {
+return "-Xloggc:" + logDirectory + "/gc.log " +
+ "-XX:+PrintGCApplicationStoppedTime " +
+ "-XX:+PrintGCDetails " +
+ "-XX:+PrintGCDateStamps " +
+ "-XX:+UseGCLogFileRotation " +
+ "-XX:NumberOfGCLogFiles=10 " +
+ "-XX:GCLogFileSize=10M " +
+ "-XX:+PrintPromotionFailure " +
+ "-XX:+PrintGCCause";
+ }
+
+ /**
+* Format the default heapdump options
+* @param appId application id
+* @param ident the ident of the process, taskmanager/jobmanager
+* @param logDirectory to print some logs
+* @param heapdumpDir to save heap dump file
+* @return the formatted heapdump options string
+*/
+ public static String getHeapdumpOpts(String appId, String ident, String
logDirectory, String heapdumpDir) {
+String dumpDestName = String.format("flink-%s-heapdump.hprof", ident);
+String dumpFileDestPath = new File(heapdumpDir, appId + "-" +
dumpDestName).getAbsolutePath();
+String oomScript = String.format("echo -e 'OutOfMemoryError! Killing
current process %%p...\n" +
+"Check gc logs and heapdump file(%s) for details.' > " +
+logDirectory + "/%s.err; " +
+"kill -9 %%p",
+ dumpFileDestPath, ident);
+return String.format("-XX:+HeapDumpOnOutOfMemoryError " +
+"-XX:HeapDumpPath=%s " +
+"-XX:OnOutOfMemoryError=\"%s\"",
+ dumpFileDestPath,
+ oomScript);
+ }
Review comment:
Same here. Please use tabs for indentation.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r341642848
##
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/clusterframework/BootstrapToolsTest.java
##
@@ -140,16 +140,38 @@ public void testSubstituteConfigKeyPrefix() {
}
}
+ public static String getGCLoggingOpts(String logDirectory) {
+ return "-Xloggc:" + logDirectory + "/gc.log " +
+ "-XX:+PrintGCApplicationStoppedTime " +
+ "-XX:+PrintGCDetails " +
+ "-XX:+PrintGCDateStamps " +
+ "-XX:+UseGCLogFileRotation " +
+ "-XX:NumberOfGCLogFiles=10 " +
+ "-XX:GCLogFileSize=10M " +
+ "-XX:+PrintPromotionFailure " +
+ "-XX:+PrintGCCause";
+ }
+
+ public static String getHeapdumpOpts(String appId, String ident, String
logDirectory, String heapdumpDir) {
+ return String.format("-XX:+HeapDumpOnOutOfMemoryError "
+
+ "-XX:HeapDumpPath=%s/%s-flink-%s-heapdump.hprof
" +
+ "-XX:OnOutOfMemoryError=\"echo -e
'OutOfMemoryError! Killing current process %%p...\nCheck gc logs and heapdump
file(%s/%s-flink-%s-heapdump.hprof) for details.' > " +
+ "%s/%s.err; kill -9 %%p\"", heapdumpDir, appId,
ident, heapdumpDir, appId, ident, logDirectory, ident);
+ }
Review comment:
I guess we could simply use the methods in the `BootstrapTools` for this.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r341645591
##
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java
##
@@ -513,6 +514,76 @@ public static Configuration
cloneConfiguration(Configuration configuration) {
return clonedConfiguration;
}
+ /**
+* Format the default gc logging options
+* @param logDirectory to save the gc log
+* @return the formatted gc logging options string
+*/
+ public static String getGCLoggingOpts(String logDirectory) {
+return "-Xloggc:" + logDirectory + "/gc.log " +
+ "-XX:+PrintGCApplicationStoppedTime " +
+ "-XX:+PrintGCDetails " +
+ "-XX:+PrintGCDateStamps " +
+ "-XX:+UseGCLogFileRotation " +
+ "-XX:NumberOfGCLogFiles=10 " +
+ "-XX:GCLogFileSize=10M " +
+ "-XX:+PrintPromotionFailure " +
+ "-XX:+PrintGCCause";
+ }
+
+ /**
+* Format the default heapdump options
+* @param appId application id
+* @param ident the ident of the process, taskmanager/jobmanager
+* @param logDirectory to print some logs
+* @param heapdumpDir to save heap dump file
+* @return the formatted heapdump options string
+*/
+ public static String getHeapdumpOpts(String appId, String ident, String
logDirectory, String heapdumpDir) {
+String dumpDestName = String.format("flink-%s-heapdump.hprof", ident);
+String dumpFileDestPath = new File(heapdumpDir, appId + "-" +
dumpDestName).getAbsolutePath();
+String oomScript = String.format("echo -e 'OutOfMemoryError! Killing
current process %%p...\n" +
+"Check gc logs and heapdump file(%s) for details.' > " +
+logDirectory + "/%s.err; " +
+"kill -9 %%p",
+ dumpFileDestPath, ident);
+return String.format("-XX:+HeapDumpOnOutOfMemoryError " +
+"-XX:HeapDumpPath=%s " +
+"-XX:OnOutOfMemoryError=\"%s\"",
+ dumpFileDestPath,
+ oomScript);
+ }
+
+ /**
+* Get the common jvm options
+* @param appId application id
+* @param ident the ident of the process, taskmanager/jobmanager
+* @param logDirectory to print some logs
+ * @param conf flink configuration
+ */
+ public static String getCommonJvmOpts(
+ String appId,
+ String ident,
+ String logDirectory,
+ Configuration conf) {
+String commonOpts = "";
+ boolean enableGCLogging =
conf.getBoolean(CoreOptions.FLINK_JVM_DEFAULT_GC_LOGGING);
+if (enableGCLogging) {
+ // Add default gc logging options if enabled
+ commonOpts += getGCLoggingOpts(logDirectory);
+}
+ boolean enableHeapDump =
conf.getBoolean(CoreOptions.FLINK_JVM_HEAPDUMP_ON_OOM);
+if (enableHeapDump) {
+ // Add default heap dump options if enabled
+ String heapdumpDir =
conf.getString(CoreOptions.FLINK_JVM_HEAPDUMP_DIRECTORY);
+ commonOpts += " " + getHeapdumpOpts(appId, ident, logDirectory,
heapdumpDir);
+}
+if (conf.getString(CoreOptions.FLINK_JVM_OPTIONS).length() > 0) {
Review comment:
```suggestion
if (!conf.getString(CoreOptions.FLINK_JVM_OPTIONS).isEmpty()) {
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r341643736
##
File path: flink-dist/src/main/flink-bin/bin/flink-daemon.sh
##
@@ -85,6 +85,13 @@ id=$([ -f "$pid" ] && echo $(wc -l < "$pid") || echo "0")
FLINK_LOG_PREFIX="${FLINK_LOG_DIR}/flink-${FLINK_IDENT_STRING}-${DAEMON}-${id}-${HOSTNAME}"
log="${FLINK_LOG_PREFIX}.log"
out="${FLINK_LOG_PREFIX}.out"
+gclog="${FLINK_LOG_PREFIX}.gc_log"
+
+FLINK_HEAPDUMP_NAME="flink-${FLINK_IDENT_STRING}-${DAEMON}-${id}-${HOSTNAME}.hprof"
+rm -rf ${FLINK_JVM_HEAPDUMP_DIRECTORY}/${FLINK_HEAPDUMP_NAME}
+FLINK_JVM_GC_LOGGING_OPTS=`getGCLoggingOpts $gclog`
+FLINK_JVM_HEAPDUMP_OPTS=`getHeapdumpOpts $FLINK_HEAPDUMP_NAME $log`
Review comment:
```suggestion
FLINK_JVM_HEAPDUMP_OPTS=$(getHeapdumpOpts $FLINK_HEAPDUMP_NAME ${log})
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r341645141
##
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java
##
@@ -513,6 +514,76 @@ public static Configuration
cloneConfiguration(Configuration configuration) {
return clonedConfiguration;
}
+ /**
+* Format the default gc logging options
+* @param logDirectory to save the gc log
+* @return the formatted gc logging options string
+*/
+ public static String getGCLoggingOpts(String logDirectory) {
+return "-Xloggc:" + logDirectory + "/gc.log " +
+ "-XX:+PrintGCApplicationStoppedTime " +
+ "-XX:+PrintGCDetails " +
+ "-XX:+PrintGCDateStamps " +
+ "-XX:+UseGCLogFileRotation " +
+ "-XX:NumberOfGCLogFiles=10 " +
+ "-XX:GCLogFileSize=10M " +
+ "-XX:+PrintPromotionFailure " +
+ "-XX:+PrintGCCause";
+ }
Review comment:
Indentation with spaces. Please use tabs.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r341643899
##
File path: flink-dist/src/main/flink-bin/mesos-bin/mesos-appmaster-job.sh
##
@@ -31,6 +31,13 @@ CC_CLASSPATH=`manglePathList
$(constructFlinkClassPath):$INTERNAL_HADOOP_CLASSPA
log="${FLINK_LOG_DIR}/flink-${FLINK_IDENT_STRING}-mesos-appmaster-${HOSTNAME}.log"
log_setting="-Dlog.file="$log"
-Dlog4j.configuration=file:"$FLINK_CONF_DIR"/log4j.properties
-Dlogback.configurationFile=file:"$FLINK_CONF_DIR"/logback.xml"
+gclog="${FLINK_LOG_DIR}/flink-${FLINK_IDENT_STRING}-mesos-appmaster-${HOSTNAME}.gc_log"
+
+FLINK_HEAPDUMP_NAME="flink-${FLINK_IDENT_STRING}-mesos-appmaster-${HOSTNAME}.hprof"
+rm -rf ${FLINK_JVM_HEAPDUMP_DIRECTORY}/${FLINK_HEAPDUMP_NAME}
+FLINK_JVM_GC_LOGGING_OPTS=`getGCLoggingOpts $gclog`
Review comment:
```suggestion
FLINK_JVM_GC_LOGGING_OPTS=$(getGCLoggingOpts ${gclog})
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r341644052
##
File path: flink-dist/src/main/flink-bin/mesos-bin/mesos-appmaster.sh
##
@@ -31,6 +31,13 @@ CC_CLASSPATH=`manglePathList
$(constructFlinkClassPath):$INTERNAL_HADOOP_CLASSPA
log="${FLINK_LOG_DIR}/flink-${FLINK_IDENT_STRING}-mesos-appmaster-${HOSTNAME}.log"
log_setting="-Dlog.file="$log"
-Dlog4j.configuration=file:"$FLINK_CONF_DIR"/log4j.properties
-Dlogback.configurationFile=file:"$FLINK_CONF_DIR"/logback.xml"
+gclog="${FLINK_LOG_DIR}/flink-${FLINK_IDENT_STRING}-mesos-appmaster-${HOSTNAME}.gc_log"
+
+FLINK_HEAPDUMP_NAME="flink-${FLINK_IDENT_STRING}-mesos-appmaster-${HOSTNAME}.hprof"
+rm -rf ${FLINK_JVM_HEAPDUMP_DIRECTORY}/${FLINK_HEAPDUMP_NAME}
+FLINK_JVM_GC_LOGGING_OPTS=`getGCLoggingOpts $gclog`
Review comment:
```suggestion
FLINK_JVM_GC_LOGGING_OPTS=$(getGCLoggingOpts ${gclog})
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r341643797
##
File path: flink-dist/src/main/flink-bin/bin/flink-daemon.sh
##
@@ -85,6 +85,13 @@ id=$([ -f "$pid" ] && echo $(wc -l < "$pid") || echo "0")
FLINK_LOG_PREFIX="${FLINK_LOG_DIR}/flink-${FLINK_IDENT_STRING}-${DAEMON}-${id}-${HOSTNAME}"
log="${FLINK_LOG_PREFIX}.log"
out="${FLINK_LOG_PREFIX}.out"
+gclog="${FLINK_LOG_PREFIX}.gc_log"
+
+FLINK_HEAPDUMP_NAME="flink-${FLINK_IDENT_STRING}-${DAEMON}-${id}-${HOSTNAME}.hprof"
+rm -rf ${FLINK_JVM_HEAPDUMP_DIRECTORY}/${FLINK_HEAPDUMP_NAME}
+FLINK_JVM_GC_LOGGING_OPTS=`getGCLoggingOpts $gclog`
Review comment:
```suggestion
FLINK_JVM_GC_LOGGING_OPTS=$(getGCLoggingOpts ${gclog})
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r339307581
##
File path: flink-dist/src/main/flink-bin/mesos-bin/mesos-taskmanager.sh
##
@@ -27,6 +27,13 @@ CC_CLASSPATH=`manglePathList
$(constructFlinkClassPath):$INTERNAL_HADOOP_CLASSPA
log=flink-taskmanager.log
log_setting="-Dlog.file="$log"
-Dlog4j.configuration=file:"$FLINK_CONF_DIR"/log4j.properties
-Dlogback.configurationFile=file:"$FLINK_CONF_DIR"/logback.xml"
+gclog="flink-taskmanager.gc_log"
+
+FLINK_HEAPDUMP_NAME="flink-taskmanager.hprof"
+rm -rf ${FLINK_JVM_HEAPDUMP_DIRECTORY}/${FLINK_HEAPDUMP_NAME}
+FLINK_JVM_GC_LOGGING_OPTS=`getGCLoggingOpts $gclog`
Review comment:
```suggestion
FLINK_JVM_GC_LOGGING_OPTS=$(getGCLoggingOpts ${gclog})
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r339307818
##
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java
##
@@ -513,6 +519,56 @@ public static Configuration
cloneConfiguration(Configuration configuration) {
return clonedConfiguration;
}
+ /**
+* Format the default gc logging options
+* @param logDirectory to save the gc log
+* @param conf flink configuration
+* @return the formatted gc logging options string
+*/
+ public static String getGCLoggingOpts(String logDirectory,
Configuration conf) {
+ boolean enableGCLogging =
conf.getBoolean(CoreOptions.FLINK_JVM_DEFAULT_GC_LOGGING);
+ if (enableGCLogging) {
+ return "-Xloggc:" + logDirectory + "/gc.log " +
+ "-XX:+PrintGCApplicationStoppedTime " +
+ "-XX:+PrintGCDetails " +
+ "-XX:+PrintGCDateStamps " +
+ "-XX:+UseGCLogFileRotation " +
+ "-XX:NumberOfGCLogFiles=10 " +
+ "-XX:GCLogFileSize=10M " +
+ "-XX:+PrintPromotionFailure " +
+ "-XX:+PrintGCCause";
+ }
+ return "";
+ }
+
+ /**
+* Format the default heapdump options
+* @param appId application id
+* @param ident the ident of the process, taskmanager/jobmanager
+* @param logDirectory to print some logs
+* @param conf flink configuration
+* @return the formatted heapdump options string
+*/
+ public static String getHeapdumpOpts(String appId, String ident, String
logDirectory, Configuration conf) {
+ boolean enableHeapDump =
conf.getBoolean(CoreOptions.FLINK_JVM_HEAPDUMP_ON_OOM);
+ String heapdumpDir =
conf.getString(CoreOptions.FLINK_JVM_HEAPDUMP_DIRECTORY);
Review comment:
I would suggest to pass in `heapdumpDir` directly because that way it is
independent of `Configuration`.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r339307656
##
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java
##
@@ -397,7 +398,12 @@ public static String getTaskManagerShellCommand(
startCommandValues.put("jvmmem", StringUtils.join(params, ' '));
- String javaOpts =
flinkConfig.getString(CoreOptions.FLINK_JVM_OPTIONS);
+ String javaOpts = "";
+ // Add default gc logging options if enabled
+ javaOpts += getGCLoggingOpts(logDirectory, flinkConfig);
+ // Add default heap dump options if enabled
+ javaOpts += " " + getHeapdumpOpts(appId, "taskmanager",
logDirectory, flinkConfig);
Review comment:
What if `getGCLoggingOpts` returns `""`? Then we would not need `" "`.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r339307874
##
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/clusterframework/BootstrapToolsTest.java
##
@@ -143,13 +143,29 @@ public void testSubstituteConfigKeyPrefix() {
@Test
public void testGetTaskManagerShellCommand() {
final Configuration cfg = new Configuration();
+ cfg.setBoolean(CoreOptions.FLINK_JVM_DEFAULT_GC_LOGGING, true);
final ContaineredTaskManagerParameters containeredParams =
new ContaineredTaskManagerParameters(1024, 768, 256, 4,
new HashMap());
// no logging, with/out krb5
final String java = "$JAVA_HOME/bin/java";
final String jvmmem = "-Xms768m -Xmx768m
-XX:MaxDirectMemorySize=256m";
+ final String defaultGCLoggingOpts =
+ "-Xloggc:./logs/gc.log " +
+ "-XX:+PrintGCApplicationStoppedTime " +
+ "-XX:+PrintGCDetails " +
+ "-XX:+PrintGCDateStamps " +
+ "-XX:+UseGCLogFileRotation " +
+ "-XX:NumberOfGCLogFiles=10 " +
+ "-XX:GCLogFileSize=10M " +
+ "-XX:+PrintPromotionFailure " +
+ "-XX:+PrintGCCause";
+ final String heapdumpOpts =
+ "-XX:+HeapDumpOnOutOfMemoryError " +
+
"-XX:HeapDumpPath=/tmp/test-flink-taskmanager-heapdump.hprof " +
+ "-XX:OnOutOfMemoryError=\"echo -e
'OutOfMemoryError! Killing current process %p...\nCheck gc logs and heapdump
file(/tmp/test-flink-taskmanager-heapdump.hprof) for details.' > " +
+ "./logs/taskmanager.err; kill -9 %p\"";
Review comment:
Can we substitute this with `getGCLoggingOpts` and `getHeapdumpOpts`?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r339307737
##
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java
##
@@ -513,6 +519,56 @@ public static Configuration
cloneConfiguration(Configuration configuration) {
return clonedConfiguration;
}
+ /**
+* Format the default gc logging options
+* @param logDirectory to save the gc log
+* @param conf flink configuration
+* @return the formatted gc logging options string
+*/
+ public static String getGCLoggingOpts(String logDirectory,
Configuration conf) {
+ boolean enableGCLogging =
conf.getBoolean(CoreOptions.FLINK_JVM_DEFAULT_GC_LOGGING);
+ if (enableGCLogging) {
Review comment:
I would move this check out of this method to simplify it. Then one would
also not need to pass in a `Configuration` object.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r339307928
##
File path:
flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java
##
@@ -1588,16 +1589,27 @@ private void
addPluginsFoldersToShipFiles(Collection effectiveShipFiles) {
}
}
- ContainerLaunchContext setupApplicationMasterContainer(
+ protected ContainerLaunchContext setupApplicationMasterContainer(
+ String appId,
String yarnClusterEntrypoint,
boolean hasLogback,
boolean hasLog4j,
boolean hasKrb5,
int jobManagerMemoryMb) {
// -- Prepare Application Master Container
--
+ String javaOpts = "";
+ // Add default gc logging options if enabled
+ javaOpts +=
BootstrapTools.getGCLoggingOpts(ApplicationConstants.LOG_DIR_EXPANSION_VAR,
flinkConfiguration);
+ // Add default heap dump options if enabled
+ javaOpts += " " + BootstrapTools.getHeapdumpOpts(
+ appId,
+ "jobmanager",
+ ApplicationConstants.LOG_DIR_EXPANSION_VAR,
+ flinkConfiguration);
// respect custom JVM options in the YAML file
- String javaOpts =
flinkConfiguration.getString(CoreOptions.FLINK_JVM_OPTIONS);
+ javaOpts += " " +
flinkConfiguration.getString(CoreOptions.FLINK_JVM_OPTIONS);
Review comment:
This looks like duplicate code which we also have in
`BootstrapTools.getTaskManagerShellCommand`. Please deduplicate.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r339307780
##
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java
##
@@ -513,6 +519,56 @@ public static Configuration
cloneConfiguration(Configuration configuration) {
return clonedConfiguration;
}
+ /**
+* Format the default gc logging options
+* @param logDirectory to save the gc log
+* @param conf flink configuration
+* @return the formatted gc logging options string
+*/
+ public static String getGCLoggingOpts(String logDirectory,
Configuration conf) {
+ boolean enableGCLogging =
conf.getBoolean(CoreOptions.FLINK_JVM_DEFAULT_GC_LOGGING);
+ if (enableGCLogging) {
Review comment:
The check could then happen in `getTaskManagerShellCommand` or in another
method `Optional maybeGetGCLoggingOpts`
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r339307589
##
File path: flink-dist/src/main/flink-bin/mesos-bin/mesos-taskmanager.sh
##
@@ -27,6 +27,13 @@ CC_CLASSPATH=`manglePathList
$(constructFlinkClassPath):$INTERNAL_HADOOP_CLASSPA
log=flink-taskmanager.log
log_setting="-Dlog.file="$log"
-Dlog4j.configuration=file:"$FLINK_CONF_DIR"/log4j.properties
-Dlogback.configurationFile=file:"$FLINK_CONF_DIR"/logback.xml"
+gclog="flink-taskmanager.gc_log"
+
+FLINK_HEAPDUMP_NAME="flink-taskmanager.hprof"
+rm -rf ${FLINK_JVM_HEAPDUMP_DIRECTORY}/${FLINK_HEAPDUMP_NAME}
+FLINK_JVM_GC_LOGGING_OPTS=`getGCLoggingOpts $gclog`
+FLINK_JVM_HEAPDUMP_OPTS=`getHeapdumpOpts $FLINK_HEAPDUMP_NAME $log`
Review comment:
```suggestion
FLINK_JVM_HEAPDUMP_OPTS=$(getHeapdumpOpts $FLINK_HEAPDUMP_NAME ${log})
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r339307942
##
File path:
flink-yarn/src/test/java/org/apache/flink/yarn/YarnClusterDescriptorTest.java
##
@@ -174,10 +174,26 @@ public void testConfigOverwrite() throws
ClusterDeploymentException {
@Test
public void testSetupApplicationMasterContainer() {
Configuration cfg = new Configuration();
+ cfg.setBoolean(CoreOptions.FLINK_JVM_DEFAULT_GC_LOGGING, true);
YarnClusterDescriptor clusterDescriptor =
createYarnClusterDescriptor(cfg);
final String java = "$JAVA_HOME/bin/java";
final String jvmmem = "-Xms424m -Xmx424m";
+ final String defaultGCLoggingOpts =
+ "-Xloggc:" + ApplicationConstants.LOG_DIR_EXPANSION_VAR
+ "/gc.log " +
+ "-XX:+PrintGCApplicationStoppedTime " +
+ "-XX:+PrintGCDetails " +
+ "-XX:+PrintGCDateStamps " +
+ "-XX:+UseGCLogFileRotation " +
+ "-XX:NumberOfGCLogFiles=10 " +
+ "-XX:GCLogFileSize=10M " +
+ "-XX:+PrintPromotionFailure " +
+ "-XX:+PrintGCCause";
+ final String heapdumpOpts =
+ "-XX:+HeapDumpOnOutOfMemoryError " +
+
"-XX:HeapDumpPath=/tmp/test-flink-jm-heapdump.hprof " +
+ "-XX:OnOutOfMemoryError=\"echo -e
'OutOfMemoryError! Killing current process %p...\nCheck gc logs and heapdump
file(/tmp/test-flink-jm-heapdump.hprof) for details.' > " +
+ "/jobmanager.err; kill -9 %p\"";
Review comment:
Can we use the helper methods to generate the expected values instead of
hard coding them here?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r339307797
##
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java
##
@@ -513,6 +519,56 @@ public static Configuration
cloneConfiguration(Configuration configuration) {
return clonedConfiguration;
}
+ /**
+* Format the default gc logging options
+* @param logDirectory to save the gc log
+* @param conf flink configuration
+* @return the formatted gc logging options string
+*/
+ public static String getGCLoggingOpts(String logDirectory,
Configuration conf) {
+ boolean enableGCLogging =
conf.getBoolean(CoreOptions.FLINK_JVM_DEFAULT_GC_LOGGING);
+ if (enableGCLogging) {
+ return "-Xloggc:" + logDirectory + "/gc.log " +
+ "-XX:+PrintGCApplicationStoppedTime " +
+ "-XX:+PrintGCDetails " +
+ "-XX:+PrintGCDateStamps " +
+ "-XX:+UseGCLogFileRotation " +
+ "-XX:NumberOfGCLogFiles=10 " +
+ "-XX:GCLogFileSize=10M " +
+ "-XX:+PrintPromotionFailure " +
+ "-XX:+PrintGCCause";
+ }
+ return "";
+ }
+
+ /**
+* Format the default heapdump options
+* @param appId application id
+* @param ident the ident of the process, taskmanager/jobmanager
+* @param logDirectory to print some logs
+* @param conf flink configuration
+* @return the formatted heapdump options string
+*/
+ public static String getHeapdumpOpts(String appId, String ident, String
logDirectory, Configuration conf) {
+ boolean enableHeapDump =
conf.getBoolean(CoreOptions.FLINK_JVM_HEAPDUMP_ON_OOM);
+ String heapdumpDir =
conf.getString(CoreOptions.FLINK_JVM_HEAPDUMP_DIRECTORY);
+ if (enableHeapDump) {
Review comment:
Same here with the check whether to return heap dump opts or not. If this
feature is not enabled, then we should not call into this method here.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r337641855
##
File path:
flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java
##
@@ -1588,16 +1589,52 @@ private void
addPluginsFoldersToShipFiles(Collection effectiveShipFiles) {
}
}
- ContainerLaunchContext setupApplicationMasterContainer(
+ protected ContainerLaunchContext setupApplicationMasterContainer(
+ String appId,
String yarnClusterEntrypoint,
boolean hasLogback,
boolean hasLog4j,
boolean hasKrb5,
int jobManagerMemoryMb) {
// -- Prepare Application Master Container
--
+ String javaOpts = "";
+ // Add default gc logging options if enabled
+ boolean enableGCLogging =
flinkConfiguration.getBoolean(CoreOptions.FLINK_JVM_DEFAULT_GC_LOGGING);
+ if (enableGCLogging) {
+ String defaultGCOptions =
+ "-Xloggc:" +
ApplicationConstants.LOG_DIR_EXPANSION_VAR + "/gc.log " +
+ "-XX:+PrintGCApplicationStoppedTime " +
+ "-XX:+PrintGCDetails " +
+ "-XX:+PrintGCDateStamps " +
+ "-XX:+UseGCLogFileRotation " +
+ "-XX:NumberOfGCLogFiles=10 " +
+ "-XX:GCLogFileSize=10M " +
+ "-XX:+PrintPromotionFailure " +
+ "-XX:+PrintGCCause";
+ javaOpts += defaultGCOptions;
+ }
+ // Add default heap dump options if enabled
+ boolean enableHeapDump =
flinkConfiguration.getBoolean(CoreOptions.FLINK_JVM_HEAPDUMP_ON_OOM);
+ String heapdumpDir =
flinkConfiguration.getString(CoreOptions.FLINK_JVM_HEAPDUMP_DIRECTORY);
+ if (enableHeapDump) {
+ String dumpFileName = "flink-jm-heapdump.hprof";
+ String dumpFileDestPath = new File(heapdumpDir, appId +
"-" + dumpFileName).getAbsolutePath();
+ String oomScript = String.format("echo -e
'OutOfMemoryError! Killing current process %%p...\n" +
+ "Check gc logs and heapdump file(%s)
for details.' > " +
+
ApplicationConstants.LOG_DIR_EXPANSION_VAR + "/jobmanager.err; " +
+ "kill -9 %%p",
+ dumpFileDestPath);
+ javaOpts += String.format(
+ " -XX:+HeapDumpOnOutOfMemoryError " +
+ "-XX:HeapDumpPath=%s " +
+ "-XX:OnOutOfMemoryError=\"%s\"",
+ dumpFileDestPath,
+ oomScript);
+ }
Review comment:
Can we deduplicate this logic wrt `BootstrapTools`?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r337641104
##
File path: flink-dist/src/main/flink-bin/mesos-bin/mesos-appmaster-job.sh
##
@@ -31,6 +31,30 @@ CC_CLASSPATH=`manglePathList
$(constructFlinkClassPath):$INTERNAL_HADOOP_CLASSPA
log="${FLINK_LOG_DIR}/flink-${FLINK_IDENT_STRING}-mesos-appmaster-${HOSTNAME}.log"
log_setting="-Dlog.file="$log"
-Dlog4j.configuration=file:"$FLINK_CONF_DIR"/log4j.properties
-Dlogback.configurationFile=file:"$FLINK_CONF_DIR"/logback.xml"
+gclog="${FLINK_LOG_DIR}/flink-${FLINK_IDENT_STRING}-mesos-appmaster-${HOSTNAME}.gc_log"
+
+if [ ${FLINK_JVM_GC_LOGGING} ];then
+FLINK_JVM_GC_LOGGING_OPTS="-XLoggc:${gclog} " \
+ "-XX:+PrintGCApplicationStoppedTime " \
+ "-XX:+PrintGCDetails " \
+ "-XX:+PrintGCDateStamps " \
+ "-XX:+UseGCLogFileRotation " \
+ "-XX:NumberOfGCLogFiles=10 " \
+ "-XX:GCLogFileSize=10M " \
+ "-XX:+PrintPromotionFailure " \
+ "-XX:+PrintGCCause"
+ JVM_ARGS="${FLINK_JVM_GC_LOGGING_OPTS} ${JVM_ARGS}"
+fi
+
+FLINK_HEAPDUMP_NAME="flink-${FLINK_IDENT_STRING}-mesos-appmaster-${HOSTNAME}.hprof"
+rm -rf ${FLINK_JVM_HEAPDUMP_DIRECTORY}/${FLINK_HEAPDUMP_NAME}
+if [ ${FLINK_JVM_HEAPDUMP_ON_OOM} ];then
+FLINK_JVM_HEAPDUMP_OPTS="-XX:+HeapDumpOnOutOfMemoryError " \
+ "-XX:HeapDumpPath=${FLINK_JVM_HEAPDUMP_DIRECTORY}/${FLINK_HEAPDUMP_NAME}
" \
+ "-XX:OnOutOfMemoryError=\"echo -e 'OutOfMemoryError! Killing current
process %p...\n" \
+ "Check gc logs and heapdump
file(${FLINK_JVM_HEAPDUMP_DIRECTORY}/${FLINK_HEAPDUMP_NAME}) for details.' >
${log}; kill -9 %p\""
+JVM_ARGS="${FLINK_JVM_HEAPDUMP_OPTS} ${JVM_ARGS}"
+fi
Review comment:
Can we deduplicate this logic? We also have it in `flink-daemon.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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add default GC options for flink on yarn to facilitate debugging
tillrohrmann commented on a change in pull request #9703: [FLINK-14038]Add
default GC options for flink on yarn to facilitate debugging
URL: https://github.com/apache/flink/pull/9703#discussion_r337642329
##
File path:
flink-yarn/src/test/java/org/apache/flink/yarn/YarnClusterDescriptorTest.java
##
@@ -174,10 +174,26 @@ public void testConfigOverwrite() throws
ClusterDeploymentException {
@Test
public void testSetupApplicationMasterContainer() {
Configuration cfg = new Configuration();
+ cfg.setBoolean(CoreOptions.FLINK_JVM_DEFAULT_GC_LOGGING, true);
YarnClusterDescriptor clusterDescriptor =
createYarnClusterDescriptor(cfg);
final String java = "$JAVA_HOME/bin/java";
final String jvmmem = "-Xms424m -Xmx424m";
+ final String defaultGCLoggingOpts =
+ "-Xloggc:" + ApplicationConstants.LOG_DIR_EXPANSION_VAR
+ "/gc.log " +
+ "-XX:+PrintGCApplicationStoppedTime " +
+ "-XX:+PrintGCDetails " +
+ "-XX:+PrintGCDateStamps " +
+ "-XX:+UseGCLogFileRotation " +
+ "-XX:NumberOfGCLogFiles=10 " +
+ "-XX:GCLogFileSize=10M " +
+ "-XX:+PrintPromotionFailure " +
+ "-XX:+PrintGCCause";
+ final String heapdumpOpts =
+ "-XX:+HeapDumpOnOutOfMemoryError " +
+
"-XX:HeapDumpPath=/tmp/test-flink-jm-heapdump.hprof " +
+ "-XX:OnOutOfMemoryError=\"echo -e
'OutOfMemoryError! Killing current process %p...\nCheck gc logs and heapdump
file(/tmp/test-flink-jm-heapdump.hprof) for details.' > " +
+ "/jobmanager.err; kill -9 %p\"";
Review comment:
Would be great to deduplicate this part as well. For example, if we have a
utility `getGCLoggingOpts()`, then we could use it here.
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:
us...@infra.apache.org
With regards,
Apache Git Services
