Skip to content

HDDS-13656. Fix aspectj settings for Java 11+#9010

Merged
adoroszlai merged 6 commits intoapache:masterfrom
symious:HDDS-13656
Sep 17, 2025
Merged

HDDS-13656. Fix aspectj settings for Java 11+#9010
adoroszlai merged 6 commits intoapache:masterfrom
symious:HDDS-13656

Conversation

@symious
Copy link
Contributor

@symious symious commented Sep 8, 2025

What changes were proposed in this pull request?

The jdk version is set to 1.8 by aspect-maven-plugin, this ticket is fix the settings related to aspectj so that we can use user specified jdk for compilation.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-13656

How was this patch tested?

MUST_HAVE=OzoneManagerStarter
javap -verbose "$(find . -type f -name ${MUST_HAVE}.class -print -quit)" 2>/dev/null | awk '/major version/{print $3; exit}'

The result should be based on "javac.version", thus not stick to 52.

@errose28 errose28 self-requested a review September 8, 2025 15:56
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @symious for the patch.

@symious
Copy link
Contributor Author

symious commented Sep 9, 2025

@adoroszlai Updated, PTAL.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @symious for updating the patch, LGTM.

Copy link
Contributor

@errose28 errose28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this @symious, I had been looking in this area well recently. Can you run a test matrix on your fork that takes each jdk version supported by our CI and runs it through the full test matrix to make sure there are no runtime issues? Although our current CI tests compile with multiple versions, the matrix is only run with the jdk21 build.

Copy link
Contributor

@errose28 errose28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, @adoroszlai would you like to take another look?

@errose28
Copy link
Contributor

Also as mentioned above, @symious it would be good to run the earlier jdk builds through the whole CI pipeline on your fork to make sure upgrade tests still run fine in those versions.

pom.xml Outdated
Comment on lines +2684 to +2685
<!-- supported since Java 9 -->
<maven.compiler.release>${javac.version}</maven.compiler.release>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also set in global properties, so I guess we can remove the java9-or-later profile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@symious
Copy link
Contributor Author

symious commented Sep 12, 2025

Also as mentioned above, @symious it would be good to run the earlier jdk builds through the whole CI pipeline on your fork to make sure upgrade tests still run fine in those versions.

@errose28 Do you mean use earlier ozone-runner like this? https://github.com/symious/ozone/actions/runs/17606869330, https://github.com/symious/ozone/actions/runs/17606886128, but the runners are out of date.

@errose28
Copy link
Contributor

Ah I didn't realize the runner only provided one jdk version. I think we should have enough coverage by just running the unit and integration tests in older jdk versions and skipping the acceptance tests.

@errose28
Copy link
Contributor

With help from cursor I tried some modified workflows that ran the integration tests in a matrix based on jdk version and the produced code and test output looks correct to me. There's some unrelated race condition in our build which I was unable to fix. Some of the splits fail with messages like

Error:  Failed to execute goal org.apache.maven.plugins:maven-dependency-plugin:3.8.1:copy-dependencies (copy-jars) on project hdds-server-scm: Artifact 'org.apache.ozone:hdds-common:jar:2.1.0-SNAPSHOT:compile' has not been packaged yet (is a directory). When used on reactor artifact, copy should be executed after packaging: see MDEP-187. -> [Help 2]

Other than that most of the runs seem to be passing.

  • This branch has changes to run the workflow with all jdk versions but built with bytecode 8
  • This branch extends it to also build with bytecode in the same version as the jdk.

I'm not sure if it's worth trying to get these to run properly or we should call it good enough and merge this.

@adoroszlai adoroszlai changed the title HDDS-13656. Fix aspectj settings for java9-or-later compilation HDDS-13656. Fix aspectj settings for Java 11+ Sep 17, 2025
@adoroszlai adoroszlai merged commit a00f6bd into apache:master Sep 17, 2025
42 checks passed
@adoroszlai
Copy link
Contributor

Thanks @symious for the patch, @errose28 for review and testing.

jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Jan 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants