Skip to content

Conversation

pan3793
Copy link
Member

@pan3793 pan3793 commented Apr 11, 2025

Description of PR

This reverts commit 49d4c73, because it breaks some public API, for example, org.apache.hadoop.yarn.server.resourcemanager.reservation.PlanFollower

The initial design of HADOOP-19447 requires YARN-11765 as a premise, but eventually, HADOOP-19447 chooses a different approach that does not require YARN-11765.

How was this patch tested?

Pass UT.

For code changes:

  • Does the title or this PR starts with the corresponding JIRA issue id (e.g. 'HADOOP-17799. Your PR title ...')?
  • Object storage: have the integration tests been executed and the endpoint declared according to the connector-specific documentation?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE, LICENSE-binary, NOTICE-binary files?

…project to hadoop-common-project for Reusability (apache#7352)  Contributed by Jiandan Yang."

This reverts commit 49d4c73.
@slfan1989
Copy link
Contributor

@pan3793 Thank you for your contribution! From my perspective, I believe we should minimize changes to multiple modules, so this PR seems reasonable to me. @yangjiandan , could you please take a look at this PR?

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 32s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 3s No case conflicting files found.
+0 🆗 codespell 0m 0s codespell was not available.
+0 🆗 detsecrets 0m 0s detect-secrets was not available.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 47 new or modified test files.
_ trunk Compile Tests _
+0 🆗 mvndep 6m 15s Maven dependency ordering for branch
+1 💚 mvninstall 31m 29s trunk passed
+1 💚 compile 15m 43s trunk passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu120.04
+1 💚 compile 13m 45s trunk passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu120.04-b06
+1 💚 checkstyle 4m 28s trunk passed
+1 💚 mvnsite 13m 25s trunk passed
+1 💚 javadoc 11m 50s trunk passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu120.04
+1 💚 javadoc 10m 21s trunk passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu120.04-b06
+1 💚 spotbugs 18m 55s trunk passed
+1 💚 shadedclient 36m 4s branch has no errors when building and testing our client artifacts.
_ Patch Compile Tests _
+0 🆗 mvndep 0m 32s Maven dependency ordering for patch
+1 💚 mvninstall 7m 12s the patch passed
+1 💚 compile 16m 11s the patch passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu120.04
+1 💚 javac 16m 11s the patch passed
+1 💚 compile 14m 9s the patch passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu120.04-b06
+1 💚 javac 14m 9s the patch passed
-1 ❌ blanks 0m 0s /blanks-eol.txt The patch has 1 line(s) that end in blanks. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
-0 ⚠️ checkstyle 4m 17s /results-checkstyle-root.txt root: The patch generated 4 new + 2017 unchanged - 0 fixed = 2021 total (was 2017)
+1 💚 mvnsite 12m 12s the patch passed
+1 💚 javadoc 10m 52s the patch passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu120.04
+1 💚 javadoc 10m 16s the patch passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu120.04-b06
+1 💚 spotbugs 22m 32s the patch passed
+1 💚 shadedclient 35m 23s patch has no errors when building and testing our client artifacts.
_ Other Tests _
+1 💚 unit 15m 7s hadoop-common in the patch passed.
+1 💚 unit 6m 12s hadoop-yarn-common in the patch passed.
+1 💚 unit 4m 56s hadoop-yarn-server-common in the patch passed.
-1 ❌ unit 807m 18s /patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt hadoop-yarn-server-resourcemanager in the patch passed.
-1 ❌ unit 1m 54s /patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager.txt hadoop-yarn-server-nodemanager in the patch failed.
+1 💚 unit 29m 4s hadoop-yarn-client in the patch passed.
+1 💚 unit 9m 3s hadoop-mapreduce-client-core in the patch passed.
-1 ❌ unit 0m 47s /patch-unit-hadoop-mapreduce-project_hadoop-mapreduce-client_hadoop-mapreduce-client-app.txt hadoop-mapreduce-client-app in the patch failed.
-1 ❌ unit 0m 44s /patch-unit-hadoop-mapreduce-project_hadoop-mapreduce-client_hadoop-mapreduce-client-hs.txt hadoop-mapreduce-client-hs in the patch failed.
-1 ❌ unit 0m 50s /patch-unit-hadoop-mapreduce-project_hadoop-mapreduce-client_hadoop-mapreduce-client-jobclient.txt hadoop-mapreduce-client-jobclient in the patch failed.
+1 💚 unit 0m 45s hadoop-yarn-server-router in the patch passed.
+1 💚 unit 22m 35s hadoop-yarn-applications-distributedshell in the patch passed.
+1 💚 unit 21m 6s hadoop-yarn-services-core in the patch passed.
+1 💚 unit 12m 46s hadoop-sls in the patch passed.
+1 💚 asflicense 1m 20s The patch does not generate ASF License warnings.
1238m 3s
Reason Tests
Failed junit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestCapacitySchedulerMultiNodes
Subsystem Report/Notes
Docker ClientAPI=1.48 ServerAPI=1.48 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-7599/1/artifact/out/Dockerfile
GITHUB PR #7599
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets
uname Linux 4c58012887f1 5.15.0-134-generic #145-Ubuntu SMP Wed Feb 12 20:08:39 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/bin/hadoop.sh
git revision trunk / 156cf8c
Default Java Private Build-1.8.0_442-8u442-b06us1-0ubuntu120.04-b06
Multi-JDK versions /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu120.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_442-8u442-b06us1-0ubuntu120.04-b06
Test Results https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-7599/1/testReport/
Max. process+thread count 1734 (vs. ulimit of 5500)
modules C: hadoop-common-project/hadoop-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-hs hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-applications-distributedshell hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services/hadoop-yarn-services-core hadoop-tools/hadoop-sls U: .
Console output https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-7599/1/console
versions git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

@yangjiandan
Copy link
Contributor

Hi @pan3793 @slfan1989 , I don’t think we need to revert YARN-11765.

First, the same concern was raised by @cnauroth in this comment before. As I mentioned in my earlier comment, our plan is to temporarily copy the clock-related classes into the hadoop-common project and remove the duplicates in a future major release.

Second, although due to design changes HADOOP-19447 no longer depends on YARN-11765, it still makes sense to have the clock-related classes in hadoop-common, as they are potentially useful across multiple Hadoop subprojects.

@pan3793
Copy link
Member Author

pan3793 commented Apr 15, 2025

@yangjiandan compatibility concerns are not the interface itself, it's about the reference.

as I mentioned in the PR description, org.apache.hadoop.yarn.server.resourcemanager.reservation.PlanFollower is a public API and references org.apache.hadoop.yarn.util.Clock, your change would break the third-party implementation that compiled with old Hadoop version.

... it still makes sense to have the clock-related classes in hadoop-common, as they are potentially useful across multiple Hadoop subprojects.

when required, you can introduce new interfaces in hadoop-common or just use java.time.Clock provided by JDK, but don't change the existing usage.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 32s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 2s No case conflicting files found.
+0 🆗 codespell 0m 1s codespell was not available.
+0 🆗 detsecrets 0m 1s detect-secrets was not available.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 47 new or modified test files.
_ trunk Compile Tests _
+0 🆗 mvndep 7m 2s Maven dependency ordering for branch
-1 ❌ mvninstall 30m 57s /branch-mvninstall-root.txt root in trunk failed.
+1 💚 compile 15m 44s trunk passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu120.04
+1 💚 compile 13m 29s trunk passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu120.04-b06
+1 💚 checkstyle 4m 26s trunk passed
+1 💚 mvnsite 13m 11s trunk passed
+1 💚 javadoc 11m 59s trunk passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu120.04
+1 💚 javadoc 11m 1s trunk passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu120.04-b06
+1 💚 spotbugs 19m 50s trunk passed
+1 💚 shadedclient 34m 41s branch has no errors when building and testing our client artifacts.
_ Patch Compile Tests _
+0 🆗 mvndep 0m 33s Maven dependency ordering for patch
+1 💚 mvninstall 7m 23s the patch passed
+1 💚 compile 15m 0s the patch passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu120.04
+1 💚 javac 15m 0s the patch passed
+1 💚 compile 13m 39s the patch passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu120.04-b06
+1 💚 javac 13m 39s the patch passed
+1 💚 blanks 0m 0s The patch has no blanks issues.
-0 ⚠️ checkstyle 4m 22s /results-checkstyle-root.txt root: The patch generated 4 new + 2017 unchanged - 0 fixed = 2021 total (was 2017)
+1 💚 mvnsite 13m 16s the patch passed
+1 💚 javadoc 11m 47s the patch passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu120.04
+1 💚 javadoc 11m 9s the patch passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu120.04-b06
+1 💚 spotbugs 22m 36s the patch passed
+1 💚 shadedclient 35m 8s patch has no errors when building and testing our client artifacts.
_ Other Tests _
+1 💚 unit 15m 9s hadoop-common in the patch passed.
+1 💚 unit 6m 10s hadoop-yarn-common in the patch passed.
+1 💚 unit 5m 1s hadoop-yarn-server-common in the patch passed.
+1 💚 unit 145m 9s hadoop-yarn-server-resourcemanager in the patch passed.
+1 💚 unit 27m 41s hadoop-yarn-server-nodemanager in the patch passed.
+1 💚 unit 29m 2s hadoop-yarn-client in the patch passed.
+1 💚 unit 9m 9s hadoop-mapreduce-client-core in the patch passed.
+1 💚 unit 9m 13s hadoop-mapreduce-client-app in the patch passed.
+1 💚 unit 4m 39s hadoop-mapreduce-client-hs in the patch passed.
+1 💚 unit 129m 26s hadoop-mapreduce-client-jobclient in the patch passed.
+1 💚 unit 1m 1s hadoop-yarn-server-router in the patch passed.
+1 💚 unit 22m 48s hadoop-yarn-applications-distributedshell in the patch passed.
+1 💚 unit 21m 25s hadoop-yarn-services-core in the patch passed.
+1 💚 unit 12m 58s hadoop-sls in the patch passed.
+1 💚 asflicense 1m 19s The patch does not generate ASF License warnings.
745m 9s
Subsystem Report/Notes
Docker ClientAPI=1.48 ServerAPI=1.48 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-7599/2/artifact/out/Dockerfile
GITHUB PR #7599
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets
uname Linux 806cae432bf7 5.15.0-134-generic #145-Ubuntu SMP Wed Feb 12 20:08:39 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/bin/hadoop.sh
git revision trunk / 9cacd55
Default Java Private Build-1.8.0_442-8u442-b06us1-0ubuntu120.04-b06
Multi-JDK versions /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu120.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_442-8u442-b06us1-0ubuntu120.04-b06
Test Results https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-7599/2/testReport/
Max. process+thread count 1570 (vs. ulimit of 5500)
modules C: hadoop-common-project/hadoop-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-hs hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-applications-distributedshell hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services/hadoop-yarn-services-core hadoop-tools/hadoop-sls U: .
Console output https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-7599/2/console
versions git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

@yangjiandan
Copy link
Contributor

Replacing the Clock interface with java.time.Clock could be a significantly larger change.
Please feel free to revert YARN-11765 if it causes issues with your development.

@slfan1989
Copy link
Contributor

@pan3793 @yangjiandan Thank you for your discussion! If there are no other comments, I will merge this PR in 1 day.

@slfan1989 slfan1989 merged commit e73722e into apache:trunk Apr 22, 2025
1 of 2 checks passed
@YanivKunda
Copy link
Contributor

@yangjiandan I've initially done most of the java.time.Clock transformation in #7570, but it was halted due to the same public breaking changes in YARN-11765 -
How about I do a more conservative version of my PR, covering only internal usages?
Is it enough to adhere to the visibility annotations for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants