gyfora commented on code in PR #561:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/561#discussion_r1156577779
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/config/FlinkConfigBuilder.java:
##########
@@ -409,15 +419,44 @@ private static void setPodTemplate(
effectiveConfig.setString(
podConfigOption,
createTempFile(
- mergePodTemplates(
- basicPod,
- appendPod,
- effectiveConfig.get(
- KubernetesOperatorConfigOptions
-
.POD_TEMPLATE_MERGE_BY_NAME))));
+ applyResourceToPodTemplate(
+ mergePodTemplates(
+ basicPod,
+ appendPod,
+ effectiveConfig.get(
+
KubernetesOperatorConfigOptions
+
.POD_TEMPLATE_MERGE_BY_NAME)),
+ resource)));
}
}
+ private static Pod applyResourceToPodTemplate(Pod podTemplate, Resource
resource) {
+ if (resource != null
+ &&
!StringUtils.isNullOrWhitespaceOnly(resource.getEphemeralStorage())
+ && podTemplate != null
+ && podTemplate.getSpec() != null) {
Review Comment:
This will ignore the ephemeralStorage setting if the user did not specify
any podTemplate (or spec in the podTemplate), that is probably incorrect.
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/config/FlinkConfigBuilder.java:
##########
@@ -409,15 +419,44 @@ private static void setPodTemplate(
effectiveConfig.setString(
podConfigOption,
createTempFile(
- mergePodTemplates(
- basicPod,
- appendPod,
- effectiveConfig.get(
- KubernetesOperatorConfigOptions
-
.POD_TEMPLATE_MERGE_BY_NAME))));
+ applyResourceToPodTemplate(
+ mergePodTemplates(
+ basicPod,
+ appendPod,
+ effectiveConfig.get(
+
KubernetesOperatorConfigOptions
+
.POD_TEMPLATE_MERGE_BY_NAME)),
+ resource)));
}
}
+ private static Pod applyResourceToPodTemplate(Pod podTemplate, Resource
resource) {
+ if (resource != null
+ &&
!StringUtils.isNullOrWhitespaceOnly(resource.getEphemeralStorage())
+ && podTemplate != null
+ && podTemplate.getSpec() != null) {
+ for (Container container : podTemplate.getSpec().getContainers()) {
Review Comment:
Should this apply for all containers or only to the main jm/tm container?
`Constants.MAIN_CONTAINER_NAME`
I think only to the main one.
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/config/FlinkConfigBuilder.java:
##########
@@ -409,15 +419,44 @@ private static void setPodTemplate(
effectiveConfig.setString(
podConfigOption,
createTempFile(
- mergePodTemplates(
- basicPod,
- appendPod,
- effectiveConfig.get(
- KubernetesOperatorConfigOptions
-
.POD_TEMPLATE_MERGE_BY_NAME))));
+ applyResourceToPodTemplate(
+ mergePodTemplates(
+ basicPod,
+ appendPod,
+ effectiveConfig.get(
+
KubernetesOperatorConfigOptions
+
.POD_TEMPLATE_MERGE_BY_NAME)),
+ resource)));
}
}
+ private static Pod applyResourceToPodTemplate(Pod podTemplate, Resource
resource) {
+ if (resource != null
+ &&
!StringUtils.isNullOrWhitespaceOnly(resource.getEphemeralStorage())
+ && podTemplate != null
+ && podTemplate.getSpec() != null) {
Review Comment:
We need some more test coverage for different configs:
1. Ephemeral storage settings without any podtemplate
2. Ephemeral storage without tm/jm podTemplate
3. Ephemeral storage without spec in podTemplate
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/config/FlinkConfigBuilder.java:
##########
@@ -409,15 +419,44 @@ private static void setPodTemplate(
effectiveConfig.setString(
podConfigOption,
createTempFile(
- mergePodTemplates(
- basicPod,
- appendPod,
- effectiveConfig.get(
- KubernetesOperatorConfigOptions
-
.POD_TEMPLATE_MERGE_BY_NAME))));
+ applyResourceToPodTemplate(
+ mergePodTemplates(
Review Comment:
I think we should not nest these calls but do one after the other for
readability
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
