[GitHub] [flink] xintongsong commented on a change in pull request #10571: [FLINK-15241] Revert the unexpected change of the configuration for M…

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

xintongsong commented on a change in pull request #10571: [FLINK-15241] Revert the unexpected change of the configuration for M…
URL: https://github.com/apache/flink/pull/10571#discussion_r358027802
 
 

 ##########
 File path: flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/TaskExecutorResourceUtils.java
 ##########
 @@ -570,7 +570,7 @@ private static CPUResource getCpuCores(final Configuration config, double fallba
 			if (cpuCores < 0) {
 				throw new IllegalConfigurationException("Configured cpu cores must be non-negative.");
 			}
-		} else if (fallback >= 0.0) {
 
 Review comment:
   I think the reason we allowed `TaskExecutorResourceUtils#getCpuCores` to return 0, is simply because 0 is a valid value for `CPUResource`. And the reason `CPUResource` accepts 0 is, as @tillrohrmann said, to allow slot requests to require no cores in their `ResourceSpec`.
   
   However, for a task executor rather than a slot request, it does not make much sense to request 0 cores. Besides, after a double checking with the previous code, I believe not allowing 0 cpu cores for a task executor is more aligned with the previous behavior (for mesos, it does not make much difference for yarn/standalone).
   
   Thus, I would agree to make `TaskExecutorResourceUtils#getCpuCores` to not allow 0 cpu cores.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services