azagrebin commented on a change in pull request #9105: 
[FLINK-13241][Yarn/Mesos] Fix Yarn/MesosResourceManager setting managed memory 
size into wrong configuration instance.
URL: https://github.com/apache/flink/pull/9105#discussion_r304309493
 
 

 ##########
 File path: 
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnConfigurationITCase.java
 ##########
 @@ -187,6 +191,12 @@ public void testFlinkContainerMemory() throws Exception {
                                        assertThat(
                                                (double) 
taskManagerInfo.getHardwareDescription().getSizeOfJvmHeap() / (double) 
expectedHeadSize,
                                                is(closeTo(1.0, 0.15)));
+
+                                       final int defaultTaskManagerMemoryMB = 
ConfigurationUtils.getTaskManagerHeapMemory(configuration).getMebiBytes();
 
 Review comment:
   Can we use a precomputed constant value for `expectedManagedMemoryMB`, the 
really expected one?
   So that the test does not depend on 
`updateTaskManagerConfigAndCreateWorkerSlotProfiles` because otherwise if there 
is an error in `updateTaskManagerConfigAndCreateWorkerSlotProfiles`, this test 
will also calculate incorrect value and pass.
   e.g. `defaultTaskManagerMemoryMB` could be set to some constant in 
`configuration` and corresponding constant `expectedManagedMemoryMB` could be 
used for the check.

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

Reply via email to