[GitHub] [flink] tillrohrmann commented on a change in pull request #10852: [FLINK-15564][yarn][test] Fix YarnClusterDescriptorTest that failed to validate the original intended behavior

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

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

 ##########
 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 agree with Andrey that overriding production code methods should always be our last resort when it comes to testing. The danger is that this method evolves and that we override important behaviour unintentionally. I would propose two solutions to the problem:
   
   1. Introduce a `YarnClusterInformationRetriever` interface which offers the method `getMaxVcores`. The default implementation will simply use the `YarnClient` to retrieve the max vcores. In the test we can provide a testing implementation.
   2. Alternatively, similar to `TestingYarnClient` override the `getNodeReports` method from `YarnClientImpl`. 

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