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:
[email protected]
With regards,
Apache Git Services
