YARN-11874. ResourceManager REST API backward compatibility with Jersey1 #8033
+8,820
−17,754
Conversation
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
|
💔 -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. |
|
@K0K0V0K Thank you for bringing this issue to our attention. I will assist in completing the upgrade for YARN-11874. |
|
💔 -1 overall
This message was automatically generated. |
|
Since Yetus cannot handle very large PRs, we need to temporarily replace it with Ayush’s branch during development. Once this PR is merged, we should revert this change. |
|
💔 -1 overall
This message was automatically generated. |
|
Hi @slfan1989 ! Huge thanks for the help! Some update:
I tried to make jettison working but i was not able to find the right configs to reproduce the Jersey1 API behaviour. |
@K0K0V0K Thank you for this PR! Let’s work together to find a solution to this issue. |
|
💔 -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. |
K0K0V0K
changed the title
|
Hi @slfan1989! I think i reached a reviewable phase with this PR. |
Thanks a lot for your great work on this! This PR is really important, and I’ll make it my top priority to follow up. |
There was a problem hiding this comment.
Thank you @K0K0V0K for raising this PR. This really help a lot!
It looks good to me but I do have some clean up comments and some follow up questions.
...r/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/dao/NodeLabelsInfo.java
Show resolved
Hide resolved
...r/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/dao/NodeLabelsInfo.java
Show resolved
Hide resolved
.../apache/hadoop/yarn/server/resourcemanager/federation/TestFederationRMStateStoreService.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServicesApps.java
Outdated
Show resolved
Hide resolved
...va/org/apache/hadoop/yarn/server/resourcemanager/webapp/helper/AppInfoJsonVerifications.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
- replace expected string to const - remove dead code
brumi1024
approved these changes
Oct 18, 2025
| YETUS='yetus' | ||
| // Branch or tag name. Yetus release tags are 'rel/X.Y.Z' | ||
| YETUS_VERSION='rel/0.14.0' | ||
| YETUS_VERSION='a7d29a6a72750a0c5c39512f33945e773e69303e' |
There was a problem hiding this comment.
Let's not forget to revert this in a follow-up, once this gets merged.
There was a problem hiding this comment.
Thanks @brumi1024 for the review!
|
@K0K0V0K Thank you for your contribution. I need some time to carefully review this PR and expect to provide a final conclusion by tomorrow. |
|
💔 -1 overall
This message was automatically generated. |
slfan1989
reviewed
Oct 19, 2025
| assertEquals(MediaType.APPLICATION_XML_TYPE + ";" + JettyUtils.UTF_8, | ||
| response.getMediaType().toString()); | ||
| String xml = response.readEntity(String.class); | ||
| System.err.println(xml); |
There was a problem hiding this comment.
This line can be removed.
slfan1989
reviewed
Oct 19, 2025
|
|
||
| public final WebTarget targetWithJsonObject() { | ||
| return target().register(new JettisonObjectProvider.App()); | ||
| return target(); |
There was a problem hiding this comment.
We introduced targetWithJsonObject() mainly to keep compatibility with existing calls, such as those in the MapReduce and NodeManager modules, which still use:
response.readEntity(JSONObject.class);
Without this method, we would have to change it to:
String res = response.readEntity(String.class);
JSONObject jsonObject = new JSONObject(res);
That would affect other modules and make the code less consistent, so keeping the original interface is a safer choice.
slfan1989
reviewed
Oct 19, 2025
| import java.net.InetSocketAddress; | ||
|
|
||
| import org.apache.hadoop.conf.Configuration; | ||
|
|
slfan1989
reviewed
Oct 19, 2025
There was a problem hiding this comment.
I’ve checked out this PR locally and overall I agree with the changes — great work! There are just a few minor points I’d like to mention:
-
I’d prefer to keep the usage of
response.readEntity(JSONObject.class);.
Perhaps we could introduce an additional reader to support this, since this approach is actually the recommended one inJersey 2. -
The JSON comparison in the resource has been nicely restored to match the original one — really appreciate that!
However, for"attributes" : "", it would be better to change it to"attributes" : {}to maintain the correct structure. -
The
targetWithJsonObjectmethod in JerseyTestBase.java is also used in MapReduce and NodeManager, so it shouldn’t be replaced directly withtarget().
We need to make sure those modules remain compatible.
| @Override | ||
| public boolean configure(FeatureContext context) { | ||
| //Auto discovery should be disabled to ensure the custom providers will be used | ||
| context.property(CommonProperties.MOXY_JSON_FEATURE_DISABLE, true); |
There was a problem hiding this comment.
Looks like 2001 and 2002 are priority values that control the provider order, right?
Maybe we could add a short comment to explain their meaning — it would make the intent clearer for future contributors.
| .get(Response.class); | ||
| assertEquals(MediaType.APPLICATION_JSON_TYPE + ";" + JettyUtils.UTF_8, | ||
| response.getMediaType().toString()); | ||
| JSONObject json = response.readEntity(JSONObject.class); |
There was a problem hiding this comment.
Any chance we can keep this pattern?
I find response.readEntity(JSONObject.class); cleaner and more intuitive — it’s also how things worked back in Jersey 1.
| "vCores" : 0, | ||
| "resourceInformations" : { | ||
| "resourceInformation" : [ { | ||
| "attributes" : "", |
There was a problem hiding this comment.
Rather than deleting "attributes": "", we should change it back to "attributes": { } to maintain the expected JSON format.
Hean-Chhinling
approved these changes
Oct 20, 2025
There was a problem hiding this comment.
LGTM with the latest changes!
Thank you @K0K0V0K for the contributions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
Description of PR
JIRA: YARN-11874
Title: ResourceManager REST API backward compatibility with Jersey1
Background
During the upgrade from Jersey1 to Jersey2, the YARN REST API message structure changed in the following ways:
• The
javax.xml.bind.annotation.XmlTypeannotation is now generated as@xsi.typeinstead oftype.• JSON arrays with a single element are rendered as a single object rather than an array containing one object.
• Every XML root element is generated by default, whereas previously it was configurable.
These changes have caused compatibility issues:
• The YARN RM UI2 cannot load properly due to the changed API behavior.
• Some third-party systems that rely on the previous API behavior may break.
Solution
This PR reverts the API behavior to be compatible with Hadoop 3.4.2 (Jersey1) API documentation:
ResourceManager REST API (Hadoop 3.4.2)
Implementation Details
• It was not possible to configure jersey-media-json-jettison to behave like Jersey1.
• Introduced a new dependency: jersey-media-moxy to restore backward-compatible behavior.
• jersey-media-moxy can be configured via the following properties:
MarshallerProperties Documentation
Testing