xintongsong commented on a change in pull request #11248: [FLINK-16299] Release
containers recovered from previous attempt in w…
URL: https://github.com/apache/flink/pull/11248#discussion_r385998143
##########
File path:
flink-yarn/src/test/java/org/apache/flink/yarn/YarnResourceManagerTest.java
##########
@@ -321,9 +322,16 @@ void runTest(RunnableWithException testMethod) throws
Exception {
}
}
- void verifyContainerHasBeenStarted(Container testingContainer) {
+ void verifyContainerHasBeenStarted(Container testingContainer,
List<ContainerId> startedContainerIds) throws Exception {
verify(mockResourceManagerClient,
VERIFICATION_TIMEOUT).removeContainerRequest(any(AMRMClient.ContainerRequest.class));
- verify(mockNMClient,
VERIFICATION_TIMEOUT).startContainerAsync(eq(testingContainer),
any(ContainerLaunchContext.class));
+ // Wait the call of
+ for (int i = 0; i < TIMEOUT.toMilliseconds(); i +=
1000) {
+ if
(startedContainerIds.contains(testingContainer.getId())) {
+ return;
+ }
+ Thread.sleep(1000);
+ }
+ throw new Exception("The container has not been start
before timeout.");
Review comment:
I see a thread safety issue here. The collection `startedContainerIds` are
concurrently accessed by this verification method and
`TestingNMClientAsync#startContainerAsyncBiConsumer`. It's probably fine at the
moment, since the verification method does not do any modifications. But I
think it would still be good to avoid this.
In the test case, for each container we could create a `CompletableFuture`,
and complete the future in
`TestingNMClientAsync#startContainerAsyncBiConsumer`. And in the verification
method, we can use `CompletableFuture#get(timeout, unit)` to verify whether the
container is started.
----------------------------------------------------------------
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
