xintongsong commented on code in PR #21565:
URL: https://github.com/apache/flink/pull/21565#discussion_r1062064266
##########
flink-yarn/src/main/java/org/apache/flink/yarn/YarnResourceManagerDriver.java:
##########
@@ -435,9 +487,19 @@ private void startTaskExecutorInContainerAsync(
containerLaunchContextFuture.handleAsync(
(context, exception) -> {
if (exception == null) {
-
nodeManagerClient.startContainerAsync(container, context);
- requestResourceFuture.complete(
- new YarnWorkerNode(container,
resourceId));
+ if (FutureUtils.isCompletedWithException(
+ requestResourceFuture,
+
ActiveResourceManager.RequestCancelledException.class)) {
+ log.info(
+ "container {} already be
cancelled.",
+ container.getId());
+
resourceManagerClient.releaseAssignedContainer(
+ container.getId());
+ } else {
+
nodeManagerClient.startContainerAsync(container, context);
+ requestResourceFuture.complete(
+ new YarnWorkerNode(container,
resourceId));
+ }
Review Comment:
Thanks for the explanation. Now I see the problem is that we selected a
`requestResourceFuture` from `requestResourceFutures` before starting the
container in the io executor, while completing the future after the io executor
finishes its job.
I think the current approach is inefficient, because a TM being started can
be canceled while there are other resource not yet being started. E.g.,
assuming we have 2 requests A and B. When there's only 1 container allocated,
we can match it to either A or B. Meantime, active resource manager decides to
cancel 1 of the 2 requests. In the current approach, it could happen that the
driver matches the allocated container to the same request (let's say A) that
the active resource manager decides to cancel. That will result in canceling of
an allocated container, while we still need to wait for another resource to be
allocated for B.
I think the desired behavior in the above example is that the active
resource manager cancels one of the request, while the driver matches the
allocated container to another request. Looking at the code,
`requestResourceFuture` is not really used after being selected until handling
the `containerLaunchContextFuture`. We may consider put off the selecting of
request resource future, so that a canceled request won't be selected. The
started container should only be discarded when there's no matching pending
requests after when handing the launch future.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
