[GitHub] [flink] xintongsong commented on a change in pull request #10608: [FLINK-15300][Runtime] Fix sanity check to not fail if shuffle memory fraction is out of min/max range

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

xintongsong commented on a change in pull request #10608: [FLINK-15300][Runtime] Fix sanity check to not fail if shuffle memory fraction is out of min/max range
URL: https://github.com/apache/flink/pull/10608#discussion_r359128206
 
 

 ##########
 File path: flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/TaskExecutorResourceUtils.java
 ##########
 @@ -551,10 +551,16 @@ private static void sanityCheckShuffleMemory(final Configuration config, final M
 					+ shuffleRangeFraction.maxSize.toString() + "].");
 			}
 			if (isShuffleMemoryFractionExplicitlyConfigured(config) &&
-				!derivedShuffleMemorySize.equals(totalFlinkMemorySize.multiply(shuffleRangeFraction.fraction))) {
-				throw new IllegalConfigurationException("Derived Shuffle Memory size("
-					+ derivedShuffleMemorySize.toString() + ") does not match configured Shuffle Memory fraction ("
-					+ shuffleRangeFraction.fraction + ").");
+				!derivedShuffleMemorySize.equals(totalFlinkMemorySize.multiply(shuffleRangeFraction.fraction)) &&
+				derivedShuffleMemorySize.getBytes() < shuffleRangeFraction.maxSize.getBytes() &&
 
 Review comment:
   I think the original implementation (`<` instead of `<=`) is correct.
   
   The purpose of this if branch is to check whether the derived size is consistent with the fraction. And the derived value is allowed to be inconsistent with the fraction iff it is capped by the min / max, and when that happens the derived size should equals to either min or max. Thus, the exception should be thrown only when the derived value is inconsistent with the fraction AND the derived value does not equals to min / max.
   
   For the purpose of guaranteeing the derived size is within the min / max range, we already have the previous if branch.
   
   Maybe we can use `!=` here for better readability and avoid confusion?

----------------------------------------------------------------
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