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