YARN-11786. Upgrade hadoop-yarn-server-timelineservice-hbase-tests to Support Trunk Compilation and Remove compatible hadoop version. #7453
+991
−296
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
|
I am attempting to remove the compatible Hadoop version for the |
|
💔 -1 overall
This message was automatically generated. |
|
I think I saw this in context of Hive as well, while trying to update Hbase, Can you check HBASE-28908, I think there is a workaround mentioned as well. See if it works |
@ayushtkn Thank you very much for the information you provided. It looks very helpful! |
slfan1989
changed the title
|
@ayushtkn @steveloughran @Hexiaoqiao @cnauroth I have completed the modifications for this module, and we should have successfully removed support for the Hadoop-compatible version. The relevant unit tests have also been validated locally. A big thanks to @ayushtkn for the valuable information—it played a crucial role in completing the upgrade of this module. The module now supports Jersey 2.46, and we've also upgraded the HBase version to 2.6.1. Could you please help review this PR? Thank you very much! |
cnauroth
reviewed
Mar 4, 2025
| @@ -150,7 +152,7 @@ public boolean equals(Object obj) { | |||
| private NavigableSet<TimelineEvent> events = new TreeSet<>(); | |||
| private HashMap<String, Set<String>> isRelatedToEntities = new HashMap<>(); | |||
| private HashMap<String, Set<String>> relatesToEntities = new HashMap<>(); | |||
| private Long createdTime; | |||
| private Long createdTime = Instant.EPOCH.toEpochMilli(); | |||
There was a problem hiding this comment.
Is this potentially a backward-incompatible change? Could we start returning "1970" where previously the value was null/missing?
There was a problem hiding this comment.
Thank you for reviewing this PR! We encountered a NullPointerException during unit testing. For the creation time, we need to set a default value, and I chose Instant.EPOCH over Instant.Min. In order to explain the issue clearly, we need to mention Jersey. I will provide a detailed explanation in response to your third question.
There was a problem hiding this comment.
I have rolled back this part of the code.
| url -> { | ||
| HttpURLConnection conn; | ||
| try { | ||
| HttpURLConnection.setFollowRedirects(false); |
There was a problem hiding this comment.
Why was it necessary to disable following redirects? This change is applied globally for the whole JVM. I wonder if it could have unintended side effects for other areas that expect to follow redirects (which is the default behavior).
There was a problem hiding this comment.
That's a good question, and you're right, it's unnecessary. I can remove the code HttpURLConnection.setFollowRedirects(false);.
| final ClientConfig cc = new ClientConfig(); | ||
| cc.connectorProvider(getHttpURLConnectionFactory()); | ||
| return ClientBuilder.newClient(cc) | ||
| .register(TimelineEntityReader.class) |
There was a problem hiding this comment.
I'm not familiar with differences in the new Jersey version, but it seems like the prior code was much more driven by introspection and annotations instead of needing to register a lot of custom readers. Is there any way to make it more like the prior code? I'm concerned about risk of backward-incompatible changes in the manual approach and potential error-prone evolution of the code if fields are changed in the future.
There was a problem hiding this comment.
This is a good question. I believe we need to update some parts of the code to align with Jersey 2's standards, as certain parts of the code are not well-supported by the latest standards.
I first became aware of the issue with Jersey during the release of Hadoop 3.4.0, when aajisaka mentioned that, with Hadoop supporting JDK 11, there was an area that needed optimization — Jersey support. For more details, you can refer to HADOOP-15984 (updating Jersey from 1.19 to 2.x).
Jersey is a framework that supports RESTful WebService. When upgrading from version 1.x to 2.x, compatibility issues arise because these two versions are completely incompatible, with even basic methods being different.
To address this, we define custom readers because Jersey 2.x only supports automatic JSON conversion for basic data types and does not support automatic conversion for more complex types. More details can be found in the official documentation.
While writing these readers, I used com.fasterxml.jackson.databind.ObjectMapper to implement automatic conversion between objects and JSON. However, ObjectMapper cannot handle null pointer exceptions, which leads to the first question — the need to assign initial values to certain variables.
After upgrading to JUnit 5, we will configure a new JDK 17 CI pipeline. Following that, I will begin the deeper adaptation and upgrade of Jersey 2.
There was a problem hiding this comment.
@slfan1989 , thank you for the reply. This is helpful information.
It sounds like some of this is just unavoidable with this Jersey upgrade. The biggest thing I'm concerned about is the potential to break YARN's user-facing APIs. Please correct me if I'm wrong, but I think we want to maintain full API compatibility in the 3.5.0 release. If so, how can we be confident about this? Maybe we need to spin up some kind of test of a 3.4 client against a 3.5 server?
There was a problem hiding this comment.
@cnauroth Thank you for your message. We need to ensure the compatibility of the interfaces. For the YARN REST API, most interfaces have no compatibility issues. Even after upgrading to Jersey 2, the changes to the server's WebService are minimal, usually just adding readers and writers. While there are still a few interfaces that need re-validation, I have a general understanding of the areas that need to be checked. We can work together to complete this task, which must be done before the release of 3.5.0.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
cnauroth
approved these changes
Mar 6, 2025
There was a problem hiding this comment.
@slfan1989 , thank you for the updates. +1 overall from me (pending pre-submit). I think it would be best to leave this open a little while longer though in case the other people you tagged want to provide feedback.
@cnauroth Thank you very much for helping to review the code! We will wait for a while to allow other members to review the PR. In the meantime, I will continue working on the JUnit5 upgrade for ResourceManager. |
|
🎊 +1 overall
This message was automatically generated. |
|
@steveloughran @ayushtkn @Hexiaoqiao Could you please take a look at this PR? Thank you so much. Apart from the checkstyle issues, we should have completed the main upgrades for this module. |
… Support Trunk Compilation and Remove compatible hadoop version.
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@ayushtkn @steveloughran @Hexiaoqiao Could you please help review this PR? Thank you very much! This is a module that must be updated during the JDK 17 upgrade process, and Yetus has already given a +1. |
74 tasks
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.
Description of PR
JIRA: YARN-11786. Upgrade hadoop-yarn-server-timelineservice-hbase-tests to Support Trunk Compilation and Remove compatible hadoop version.
I am attempting to upgrade the YARN module to JUnit5 unit tests. While working on the
hadoop-yarn-server-timelineservice-hbase-testsmodule, I discovered that it is using Hadoop version 3.3.6. I plan to unify its version to the trunk version.How was this patch tested?
mvn clean package -DskipTests -Pdist -Dmaven.javadoc.skip
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?