gyfora commented on code in PR #487:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/487#discussion_r1058430979
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/validation/DefaultValidator.java:
##########
@@ -241,15 +245,35 @@ private Optional<String> validateJobSpec(
}
private Optional<String> validateJmSpec(JobManagerSpec jmSpec, Map<String,
String> confMap) {
+ Configuration conf = Configuration.fromMap(confMap);
+ var jmMemoryDefined =
+ jmSpec != null
+ && jmSpec.getResource() != null
+ &&
!StringUtils.isNullOrWhitespaceOnly(jmSpec.getResource().getMemory());
+ Optional<String> jmMemoryValidation =
+ jmMemoryDefined ? Optional.empty() :
validateJmMemoryConfig(conf);
+
if (jmSpec == null) {
- return Optional.empty();
+ return jmMemoryValidation;
}
return firstPresent(
+ jmMemoryValidation,
validateResources("JobManager", jmSpec.getResource()),
validateJmReplicas(jmSpec.getReplicas(), confMap));
}
+ private Optional<String> validateJmMemoryConfig(Configuration conf) {
+ try {
+
JobManagerProcessUtils.processSpecFromConfigWithNewOptionToInterpretLegacyHeap(
+ conf, JobManagerOptions.JVM_HEAP_MEMORY);
+ } catch (IllegalConfigurationException e) {
Review Comment:
I think we should catch any exception here in case Flink throws something
else.
##########
flink-kubernetes-operator/src/test/java/org/apache/flink/kubernetes/operator/validation/DefaultValidatorTest.java:
##########
@@ -248,13 +252,40 @@ public void testValidationWithoutDefaultConfig() {
testError(
dep ->
dep.getSpec().getJobManager().getResource().setMemory("invalid"),
"JobManager resource memory parse error");
-
testError(
dep ->
dep.getSpec().getTaskManager().getResource().setMemory(null),
- "TaskManager resource memory must be defined");
+ "TaskManager memory configuration failed");
Review Comment:
I think the previous error message was much more informative for the user.
"TaskManager memory configuration failed" is pretty hard to understand.
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/validation/DefaultValidator.java:
##########
@@ -261,16 +285,40 @@ private Optional<String> validateJmReplicas(int replicas,
Map<String, String> co
return Optional.empty();
}
- private Optional<String> validateTmSpec(TaskManagerSpec tmSpec) {
+ private Optional<String> validateTmSpec(TaskManagerSpec tmSpec,
Map<String, String> confMap) {
+ Configuration conf = Configuration.fromMap(confMap);
+
+ var tmMemoryDefined =
+ tmSpec != null
+ && tmSpec.getResource() != null
+ &&
!StringUtils.isNullOrWhitespaceOnly(tmSpec.getResource().getMemory());
+ Optional<String> tmMemoryConfigValidation =
+ tmMemoryDefined ? Optional.empty() :
validateTmMemoryConfig(conf);
+
if (tmSpec == null) {
- return Optional.empty();
+ return tmMemoryConfigValidation;
}
+ return firstPresent(
+ tmMemoryConfigValidation,
+ validateResources("TaskManager", tmSpec.getResource()),
+ validateTmReplicas(tmSpec));
+ }
+
+ private Optional<String> validateTmMemoryConfig(Configuration conf) {
+ try {
+ TaskExecutorProcessUtils.processSpecFromConfig(conf);
+ } catch (IllegalConfigurationException e) {
Review Comment:
I think we should catch any exception here in case Flink throws something
else.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
