xintongsong commented on a change in pull request #10852:
[FLINK-15564][yarn][test] Fix YarnClusterDescriptorTest that failed to validate
the original intended behavior
URL: https://github.com/apache/flink/pull/10852#discussion_r367211936
##########
File path:
flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java
##########
@@ -318,6 +308,20 @@ private void isReadyForDeployment(ClusterSpecification
clusterSpecification) thr
}
}
+ @VisibleForTesting
+ protected int getNumYarnMaxVcores() throws YarnDeploymentException {
Review comment:
I'm not sure about this.
`YarnClient` is an abstract class, and to introduce a `StubYarnClientImpl`
we would need to also implement more than 20 useless methods and mock the
`NodeReport` as well. The complication seems unnecessary to me.
It is true the current approach touches the production class
`YarnClusterDescriptor`, but only with a trivial refactor. IMO, even without
this testability issue, it is not a bad thing to extract the logic getting max
vcores from Yarn into a separate method.
----------------------------------------------------------------
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:
us...@infra.apache.org
With regards,
Apache Git Services
