[GitHub] [flink-kubernetes-operator] gyfora commented on a diff in pull request #162: [FLINK-27029] DeploymentValidator should take default flink config into account during validation

[GitBox <gi...@apache.org>]

gyfora commented on code in PR #162:
URL: https://github.com/apache/flink-kubernetes-operator/pull/162#discussion_r846826594


##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/FlinkConfigBuilder.java:
##########
@@ -241,7 +244,9 @@ private static void setResource(
                     isJM
                             ? KubernetesConfigOptions.JOB_MANAGER_CPU
                             : KubernetesConfigOptions.TASK_MANAGER_CPU;
-            effectiveConfig.setString(memoryConfigOption.key(), resource.getMemory());
+            if (!StringUtils.isNullOrWhitespaceOnly(resource.getMemory())) {

Review Comment:
   We should not allow empty settings, this should be validated instead



##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/FlinkConfigBuilder.java:
##########
@@ -184,8 +184,11 @@ public FlinkConfigBuilder applyJobOrSessionSpec() throws URISyntaxException {
         if (spec.getJob() != null) {
             effectiveConfig.set(
                     DeploymentOptions.TARGET, KubernetesDeploymentTarget.APPLICATION.getName());
-            final URI uri = new URI(spec.getJob().getJarURI());
-            effectiveConfig.set(PipelineOptions.JARS, Collections.singletonList(uri.toString()));
+            if (spec.getJob().getJarURI() != null) {
+                effectiveConfig.set(
+                        PipelineOptions.JARS,
+                        Collections.singletonList(new URI(spec.getJob().getJarURI()).toString()));
+            }

Review Comment:
   What's the point of this change?



##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/validation/DefaultValidator.java:
##########
@@ -58,6 +58,7 @@
     @Override
     public Optional<String> validateDeployment(FlinkDeployment deployment) {
         FlinkDeploymentSpec spec = deployment.getSpec();
+        Map<String, String> effectiveConfig = FlinkUtils.getEffectiveConfig(deployment).toMap();

Review Comment:
   I think we cannot get the `getEffectiveConfig` here as that relies on the FlinkConfigBuilder logic which in turn relies on the validation. 
   
   We should imply get the defaultConfiguration I think.



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org