Uh oh!
There was an error while loading. Please reload this page.
TEZ-4580: Slow preemption of new containers when re-use is enabled - #374
Merged
Conversation
tez-yetus
commented
Sep 25, 2024
🎊 +1 overall
This message was automatically generated. |
| if (delayedContainers.remove(container)) { | ||
| LOG.info("Removed {} from delayed containers", container.getContainer().getId()); | ||
| } else { | ||
| LOG.warn("Unknown container {} sent for removal. Ignoring.", container.getContainer().getId()); |
ContributorAuthor
There was a problem hiding this comment.
A valid case here is when a new container is allocated - added to delayedContainers, it is polled (removed) from queue but if there is no pending task request releaseContainer() is invoked. Please suggest if the log level should be changed in info or for both the newly added logs should be at debug.
Member
There was a problem hiding this comment.
nit: I think we should change the log level to debug and print the only if LOG.isDebugEnabled() is true,
Rest of the code LGTM .
Contributor
There was a problem hiding this comment.
not sure about the log levels, let's keep the simple remove path on debug, as looks like the happy preemption path:
LOG.debug("Removed {} from delayed containers", container.getContainer().getId());
isDebugEnabled is not necessary as long as we use the {} formatting (to prevent unnecessary string creation)
prabhjyotsingh added a commit
to acceldata-io/tez
that referenced
this pull request
Nov 14, 2024
prabhjyotsingh added a commit
to acceldata-io/tez
that referenced
this pull request
Nov 20, 2024
…nmerged) apache#374 (cherry picked from commit aea86ca)
prabhjyotsingh added a commit
to acceldata-io/tez
that referenced
this pull request
Nov 20, 2024
…enabled (unmerged) apache#374 (#24) (cherry picked from commit aea86ca)
shubhluck pushed a commit
to acceldata-io/tez
that referenced
this pull request
Nov 21, 2024
…enabled (unmerged) apache#374 (#24) (cherry picked from commit aea86ca)
Member
@abstractdog can you please take a look when you are free. Thanks in advance! :) |
| } | ||
| void removeDelayedContainer(HeldContainer container) { | ||
| if (container != null) { |
Contributor
There was a problem hiding this comment.
this null check is not needed, as it's called from a block
if (delayedContainer != null) {
abstractdog
commented
Dec 21, 2024
Contributor
thanks @himanshu-mishra for taking care of this according to the description of TEZ-1742: after checking and trying out the unit test and the fix here, it seems like this patch what's exactly fixes the abovementioned problem apart from the assertion error fix, the unit test indeed proves that releaseAssignedContainer is called as many times as needed according to the percentage config in a single round, so this looks good to me only left a minor comment regarding a null check |
himanshu-mishra
commented
Dec 23, 2024
ContributorAuthor
Thanks @simhadri-g , @abstractdog for review. I have incorporated feedback as following:
Also adding a note that as the the changes fixes slow preemption, it leads to performance gain mentioned in TEZ-1742 when container re-use is enabled, which is enabled by default in Tez. |
tez-yetus
commented
Dec 23, 2024
🎊 +1 overall
This message was automatically generated. |
tez-yetus
commented
Dec 23, 2024
🎊 +1 overall
This message was automatically generated. |
abstractdog
commented
Dec 23, 2024
Contributor
@himanshu-mishra : minor checkstyle left, can you please address: |
tez-yetus
commented
Dec 23, 2024
🎊 +1 overall
This message was automatically generated. |
abstractdog
self-requested a review
December 23, 2024 09:29
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When container reuse is enabled, preemption of lower priority containers that are not yet assigned to task, takes long time as they are released one at a time, and not the number of containers based when tez.am.preemption.percentage is high added in https://issues.apache.org/jira/browse/TEZ-1742.
Further investigation lead to following conclusion:
Warn log / Assertion error thrown because in preemptIfNeeded(), when releasing new containers, the loop counter is being decremented with each
releaseUnassignedContainers, leading to looping only half number of times. By using another counter, assertion passes because of condition method returns with checkif (numPendingRequestsToService < 1) {.In releaseContainer(), the container is not getting removed from
delayedContainersqueue and only fromheldContainersmap, hence same container is being picked up for release in every iteration till next cycle ofDelayedContainerManagerfinds out that the container is not inheldContainersand skips it with logSkipping delayed container as container is no longer running, containerId=...This change adds a method in
DelayedContainerManagerto allow removal of delayed container and invokes it in releaseContainer method, which so far only removed it fromheldContainersmap.