Repository navigation
YARN-12005. Fix flaky TestRouterWebServicesREST#testUpdateNodeResource - #8788
Merged
Merged
Conversation
|
💔 -1 overall
This message was automatically generated. |
This was referenced Oct 4, 2026
|
💔 -1 overall
This message was automatically generated. |
slfan1989
requested changes
Oct 7, 2026
slfan1989
left a comment
slfan1989
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the fix! Waiting for the node resource update to take effect before checking the response matches the RM’s asynchronous behavior. The approach looks reasonable.
The target tests passed. The remaining Jenkins failure is a container-count comparison race in testGetAppsMultiThread. Since #8787 and #8791 have been merged, please rebase onto the latest trunk and rerun CI.
One small suggestion: please close the first POST Response in a finally block before replacing it with the second response. Also consider configuring HTTP connect and read timeouts, since the five-second eventually deadline does not bound an individual blocking request.
AdminService#updateNodeResource only dispatches an RMNodeResourceUpdateEvent on the RM's async dispatcher, and RMWebServices#updateNodeResource returns rmNode.getTotalCapability() right after dispatching. When the dispatcher is busy the POST response and the following GET can still report the old total resource (expected: <4096> but was: <8192>). Wait (bounded at 5s) until the node reports the new memory and vcores, then check the Router's POST response with a repeated, idempotent update that reads the already applied value. Contains content generated by Claude Code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Close both POST responses in testUpdateNodeResource in a finally block, and give the test's REST clients connect and read timeouts, so a stuck request fails instead of hanging past the eventually deadline. Contains content generated by Claude Code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SmHy1KaQ8NVg2X4e6CxbEH
|
🎊 +1 overall
This message was automatically generated. |
joseluisll
commented
Oct 7, 2026
joseluisll
left a comment
joseluisll
left a comment
Contributor
Author
There was a problem hiding this comment.
@slfan1989 Addressed the comments in your review, set two timeouts with constants.
slfan1989
approved these changes
Oct 7, 2026
slfan1989
left a comment
slfan1989
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Thanks for addressing the response cleanup and HTTP timeout comments. The latest Yetus run is green, and GitHub Actions Build passed. Ready to merge.
Contributor
|
@joseluisll Thanks for the contribution! Merged into trunk. |
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Depends on (has to be merged first):
clears a CI failure: it edits TestDFSClientRetries.java:1309-1328, where TestDFSClientRetries fails at line 1326 in this PR's precommit, still red in the latest run
clears a CI failure: it changes McpHttpServlet, which carries the trunk spotbugs warning SE_BAD_FIELD that turns this PR red, still red in the latest run
clears a CI failure: it edits TestStandbyCheckpoints.java:764-776, where TestStandbyCheckpoints fails at line 772 in this PR's precommit, still red in the latest run
Description of PR
JIRA: YARN-12005. Fix flaky TestRouterWebServicesREST#testUpdateNodeResource
TestRouterWebServicesREST#testUpdateNodeResource(also inherited byTestFederationWebApp) fails intermittently with:The RM applies a node resource update asynchronously.
AdminService#updateNodeResourceonly dispatches anRMNodeResourceUpdateEvent, and the new total is set later inRMNodeImpl.UpdateNodeResourceWhenRunningTransition.RMWebServices#updateNodeResourcereturnsrmNode.getTotalCapability()right after dispatching. So when the dispatcher is busy, the POST response and the following GET still report the old 8192 MB.Test-only fix:
LambdaTestUtils.eventually) until a GET of the node reports the new memory and vcores.The test has no
@Timeout, so the bounded wait is safe.Related: YARN-12004 / #8786 fixes the Router-vs-RM application lifecycle races in the same classes. It does not touch
testUpdateNodeResource, so the two changes are independent.How was this patch tested?
@RepeatedTestsubclass (not committed), with 8 threads submitting applications to the RM in the background:TestRouterWebServicesRESTandTestFederationWebApp4 times.testUpdateNodeResourcepassed in every run of both classes. The remaining intermittent failures in those classes (testAppAttemptXML,testAppXML,testGetContainersXML) also fail without this change and are covered by YARN-12004.For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles? (N/A)AI Tooling
Contains content generated by Claude Code.