YARN-6667. Handle containerId duplicate without failing the heartbeat in Federation Interceptor. #4810
Conversation
This file contains 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
added 3 commits
August 26, 2022 19:59
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Can you help review the code? Thank you very much! |
goiri
reviewed
Aug 27, 2022
| LOG.info("Use Previous Allocated Container's subCluster. " + | ||
| "ContainerId: {} ApplicationId: {} From RM: {}.", this.attemptId, | ||
| container.getId(), existingSubClusterId); | ||
| continue; |
There was a problem hiding this comment.
At the end of the for loop, there is a piece of code like this
this.containerIdToSubClusterIdMap.put(container.getId(), subClusterId);
If a container uses the previous subcluster, it does not need to execute this line, so use continue, jump out of the loop, and let the for loop continue to execute the next round.
This code may not be easy to read, I will refactor this part of the code.
|
|
||
| // If the previous RM which allocated Container is normal, | ||
| // the previous RM will be used first | ||
| if ((existAllocatedScHealth && !newAllocatedScHealth) || |
There was a problem hiding this comment.
Isn't this condition repetitive? Wouldn't it just be if (existAllocatedScHealth) ?
There was a problem hiding this comment.
Thanks for your suggestion, your suggestion is correct, I will modify the code.
| boolean newAllocatedScHealth = true; | ||
|
|
||
| // Previous SubCluster Time Out Or Unusable, Can't Continue to use. | ||
| if (timeOutScs.contains(existingSubClusterId) || |
There was a problem hiding this comment.
I think we could unify these two ifs a little.
There was a problem hiding this comment.
Thank you very much for helping to review the code, I will modify the code.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Please help to review the code again, Thank you very much! |
goiri
reviewed
Aug 29, 2022
| Set<SubClusterId> timeOutScs = getTimedOutSCs(true); | ||
| SubClusterInfo subClusterInfo = federationFacade.getSubCluster(subClusterId); | ||
| if (timeOutScs.contains(subClusterId) || | ||
| subClusterInfo == null || subClusterInfo.getState().isUnusable()) { |
| SubClusterInfo subClusterInfo = federationFacade.getSubCluster(subClusterId); | ||
| if (timeOutScs.contains(subClusterId) || | ||
| subClusterInfo == null || subClusterInfo.getState().isUnusable()) { | ||
| isSCHealth = false; |
There was a problem hiding this comment.
I would just do return true and return false at the end.
There was a problem hiding this comment.
Thanks for your suggestion, I will modify the code.
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Please help to review the code again, Thank you very much! |
goiri
approved these changes
Aug 30, 2022
|
🎊 +1 overall
This message was automatically generated. |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
… in Federation Interceptor. (apache#4810)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
JIRA: YARN-6667. Handle containerId duplicate without failing the heartbeat in Federation Interceptor.
From the actual situation, the probability of this happening is very low.
It can only be caused by the master-slave fail-hover of YARN and the wrong Epoch parameter configuration.
We will try to be compatible with this situation and let the Application run as much as possible, using the following measures: