YARN-11765. Refactor: Move Clock Class from hadoop-mapreduce-project to hadoop-common-project for Reusability #7352
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
|
@yangjiandan Thank you for your contribution! The jdiff file cannot be modified as it is the XML file used to mark API changes during the release version. |
|
@slfan1989 Thank you for the clarification! I will adjust my changes accordingly. |
|
💔 -1 overall
This message was automatically generated. |
cnauroth
reviewed
Feb 5, 2025
There was a problem hiding this comment.
@yangjiandan , that's a nice refactor!
I'm sorry to say it, but I'm not sure we can proceed with this. The YARN classes are annotated as @Public, and it would be a backward-incompatible change.
https://hadoop.apache.org/docs/current/hadoop-project-dist/hadoop-common/Compatibility.html#Java_API
I know it is used in at least Hive and Tez:
We could theoretically make a change like this in a 4.0.0 release, but I haven't seen any plans for a major version release like that.
|
💔 -1 overall
This message was automatically generated. |
|
@cnauroth Thank you for your review and the detailed explanation! To address your concern, I propose the following solution:
|
slfan1989
reviewed
Feb 6, 2025
...arn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ProcfsBasedProcessTree.java
Outdated
Show resolved
Hide resolved
slfan1989
reviewed
Feb 6, 2025
8000
...rn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/WindowsBasedProcessTree.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
b69ddd2 is my lastest version, and hadoop-yetus has +1. @slfan1989 Could you please review the code again? Let me know if any further changes are needed. Thanks! |
Thank you for your contribution! I need some time to review this PR. |
b69ddd2 to
ee885ef
Compare
|
The community trunk branch has introduced JUnit 5-related changes, which conflicted with this modification. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
yangjiandan
pushed a commit
to yangjiandan/hadoop
that referenced
this pull request
Feb 13, 2025
…to hadoop-common-project for Reusability apache#7352
299939a to
808c339
Compare
|
🎊 +1 overall
This message was automatically generated. |
cnauroth
reviewed
Feb 13, 2025
There was a problem hiding this comment.
Hello @yangjiandan . I entered one more comment.
As this is a public API change, I'd like to make sure there is enough community awareness and consensus. I'll start a thread on yarn-dev@hadoop.apache.org.
| return INSTANCE; | ||
| } | ||
|
|
||
| @Deprecated |
There was a problem hiding this comment.
Is it possible to remove this and make the constructor private? It would be unusual to introduce a new API with one of its methods immediately deprecated like this.
There was a problem hiding this comment.
@cnauroth Thanks for your review! I looked into the commit history and found that this change was introduced in YARN-4816 to fix a compatibility issue. Theoretically, this public constructor is also exposed as part of the API interface. So, to ensure compatibility, it would be safer to keep this change for now.
There was a problem hiding this comment.
The change proposed here introduces a new API in hadoop-common and advocates for callers to move to the new API. Since it is a brand new API with no pre-existing callers, we don't need to support the pre-existing interface (which was deemed deprecated).
There was a problem hiding this comment.
@cnauroth Thanks for your feedback! I understand your point that this is a new API.
I will update patch accordingly based on your suggestion.
cnauroth
reviewed
Feb 13, 2025
...-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/Clock.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
running test failed with exit code 143, retrigger by empty commit |
<
C403
span class="TimelineItem-badge">
yangjiandan
pushed a commit
to yangjiandan/hadoop
that referenced
this pull request
Mar 4, 2025
…to hadoop-common-project for Reusability apache#7352
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@steveloughran Do you have any further comments on this PR? If not, can we merge it so that @yangjiandan can proceed with the next steps? |
added 12 commits
March 7, 2025 14:47
…to hadoop-common-project for Reusability apache#7352
|
rebase trunk to trigger build |
|
🎊 +1 overall
This message was automatically generated. |
|
@cnauroth @steveloughran I plan to merge this PR. I had an offline discussion with yangjiandan, and some of his suggestions are very helpful for optimizing YARN's scheduling performance. I believe we should support him in completing this feature so he can proceed with his subsequent work. I will provide further details in an email shortly. If any issues arise from this PR in the future, we will respond promptly and fix them. cc: @yangjiandan |
|
@yangjiandan Thanks for the contribution! Merged into trunk. @cnauroth @steveloughran Thanks for the review! |
YanivKunda
pushed a commit
to YanivKunda/hadoop
that referenced
this pull request
Mar 23, 2025
…to hadoop-common-project for Reusability (apache#7352) Contributed by Jiandan Yang. Refactor: Move Clock Class from hadoop-mapreduce-project to hadoop-common-project for Reusability Reviewed-by: Chris Nauroth <cnauroth@apache.org> Reviewed-by: Steve Loughran <stevel@apache.org> Signed-off-by: Shilun Fan <slfan1989@apache.org>
|
I've noticed this work on moving Clock while I was working on HADOOP-19454, a modernization effort I recently started. |
3 tasks
pan3793
added a commit
to pan3793/hadoop
that referenced
this pull request
Apr 11, 2025
…project to hadoop-common-project for Reusability (apache#7352) Contributed by Jiandan Yang." This reverts commit 49d4c73.
slfan1989
pushed a commit
that referenced
this pull request
Apr 22, 2025
…project to hadoop-common-project for Reusability (#7352) Contributed by Jiandan Yang." (#7599) Contributed by Cheng Pan. * Revert "YARN-11765. Refactor: Move Clock Class from hadoop-mapreduce-project to hadoop-common-project for Reusability (#7352) Contributed by Jiandan Yang." Signed-off-by: Shilun Fan <slfan1989@apache.org>
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
see https://issues.apache.org/jira/browse/YARN-11765
How was this patch tested?
current ut