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

[GitBox]

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

 ##
 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 also looked into `YarnClientImpl`. My only concern about this option was 
that it is annotated  with `@Private` and `@Unstable` but I did not see that we 
actually already decided once to extend it. I would be ok with it. I guess we 
will refactor it if this class is gone after updating the Yarn dependency. 


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

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

[GitBox]

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

 ##
 File path: 
flink-yarn/src/test/java/org/apache/flink/yarn/YarnClusterDescriptorTest.java
 ##
 @@ -104,9 +104,7 @@ public void testFailIfTaskSlotsHigherThanMaxVcores() 
throws ClusterDeploymentExc
clusterDescriptor.setLocalJarPath(new Path(flinkJar.getPath()));
 
ClusterSpecification clusterSpecification = new 
ClusterSpecification.ClusterSpecificationBuilder()
-   .setMasterMemoryMB(1)
-   .setTaskManagerMemoryMB(1)
-   .setNumberTaskManagers(1)
+   .setTaskManagerMemoryMB(1024)
 
 Review comment:
   Should we actually try to change the default 
`ClusterSpecificationBuilder.taskManagerMemoryMB` to `1024` if it is already 
clear that it is inconsistent in general? This would also give us more 
confidence that what we are fixing now does not happen in other tests as well.


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

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

[GitBox]

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

 ##
 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 am wondering whether it is actually less invasive to introduce a 
`StubYarnClientImpl` and use in `YarnClusterDescriptorTest#setup` instead of 
changing production class `YarnClusterDescriptor` for this:
   ```
   class StubYarnClientImpl() extends YarnClient {
@Override
public List getNodeReports(NodeState... 
states) {
return Collections.singletonList(new 
NodeReport() {
@Override
public Resource getCapability() {
return new Resource() {
@Override
public int 
getVirtualCores() {
return 
NUM_YARN_MAX_VCORES;
}
// ...
};
}
// ...
});
}
   // ...
   }
   
   public class YarnClusterDescriptorTest extends TestLogger {
   // ..
   @BeforeClass
public static void setupClass() {
yarnConfiguration = new YarnConfiguration();
yarnClient = new StubYarnClientImpl();
yarnClient.init(yarnConfiguration);
yarnClient.start();
}
   //..
   }
   ```
   
   It is quite some useless code but we can put into a separate file.


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