feat(build): bump Scala/Java services to Java 17 LTS#4938
feat(build): bump Scala/Java services to Java 17 LTS#4938bobbai00 merged 22 commits intoapache:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4938 +/- ##
============================================
- Coverage 42.72% 42.71% -0.02%
+ Complexity 2185 2184 -1
============================================
Files 1031 1031
Lines 38152 38152
Branches 4004 4004
============================================
- Hits 16302 16298 -4
- Misses 20831 20836 +5
+ Partials 1019 1018 -1
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
25ae13f to
64ffded
Compare
aglinxinyuan
left a comment
There was a problem hiding this comment.
Do we have to include it as part of the release? If it's not required, let's not include it in the release and do more testing.
The discussion also mentioned that we need to have JDK_JAVA_OPTIONS. Is that still required? I don't see it anywhere in this PR.
### What changes were proposed in this PR? Backport #4549 (initial AGENTS.md + CLAUDE.md) and #4825 (AGENTS.md rewrite) to `release/v1.1.0-incubating`. Two cherry-picks: #4549 applies clean; #4825 has trivial content conflicts that resolve by taking the post-rewrite state (i.e. `upstream/main:AGENTS.md` verbatim — what the squashed result of both PRs would produce on main). ### Any related issues, documentation, discussions? Backport of #4549 and #4825. Unblocks the auto-backport simulation for #4938 (Java 17 bump) and any future PR that touches `AGENTS.md`. ### How was this PR tested? Doc-only change. Verified `diff <(git show HEAD:AGENTS.md) <(git show upstream/main:AGENTS.md)` is empty. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 4.7) --------- Co-authored-by: Xinyuan Lin <xinyual3@uci.edu> Co-authored-by: Yicong Huang <17627829+Yicong-Huang@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Got it. I mentioned it in the PR description and The main reason I want to include it in the release branch is because Java 11 already doesn't have an LTS version. Since we use Java as the runtime image, it would be great to have an LTS base image as the runtime. |
64ffded to
c16f305
Compare
Java 11 is extended to be supported till 2032. https://www.oracle.com/java/technologies/java-se-support-roadmap.html Do we HAVE to upgrade before release?? |
c16f305 to
18f8813
Compare
Sorry, I missed the JDK_JAVA_OPTIONS part. |
fc43932 to
d7b171c
Compare
We don't have to. I removed the tag. |
|
How about bash script in bin/? do they need to be updated for |
3563991 to
40e94aa
Compare
aglinxinyuan
left a comment
There was a problem hiding this comment.
Do you want to add that to the IntelliJ configuration in the .run folder so developers can also run with JDK_JAVA_OPTIONS on their local machines? Please also don't solely rely on test cases for this big change. Let's run a real workflow to manually test it.
Added |
During my daily development, I use Java 17 all the time and manually ran lots of workflows. So I am pretty confident with the changes. To be safe, can you try this branch and manually run some workflows to test it? |
Forking changed the test JVM's CWD to the subproject directory, so tests reading repo-root-relative paths (e.g. MockTexeraDB -> sql/texera_ddl.sql) failed with NoSuchFileException. Set ThisBuild / Test / baseDirectory back to the build root.
Earlier commits introduced Test / fork := true and a JdkOptions plugin to inject JDK 17 --add-opens flags. Forking changed the test JVM's CWD to each subproject directory, which broke tests that read repo-root-relative paths (sql/texera_ddl.sql from MockTexeraDB), and required a baseDirectory pin to fix. Replace all of that with a single .jvmopts at the repo root: sbt's launcher reads it and applies the flags to sbt's own JVM startup. With Test / fork at its default (false), tests run in sbt's JVM and inherit the flags — no fork overhead, no CWD change, no baseDirectory pin, no sbt plugin file. Drops: - project/JdkOptions.scala - ThisBuild / Test / fork := true - ThisBuild / Test / javaOptions ++= ... - ThisBuild / Test / baseDirectory := ... Adds: - .jvmopts (5 --add-opens lines, attribution comments)
# Conflicts: # bin/computing-unit-master.dockerfile # bin/computing-unit-worker.dockerfile
|
The amber run again hangs and drains our CI. canceled manually. please check https://github.com/apache/texera/actions/runs/25420608272/job/74561719299?pr=4938 @bobbai00 @aglinxinyuan |
…telliJ
The amber CI job hung at the 6h timeout because sbt-jacoco's forked
test JVM did not inherit .jvmopts (which is read only by sbt's own
launcher). On JDK 17, Pekko's serializer reflected on
java.lang.invoke.SerializedLambda and threw InaccessibleObjectException;
tests waited on the resulting unresolved future and never exited.
Make .jvmopts the single source of truth, consumed by:
- sbt's own JVM (already)
- forked test JVMs (ThisBuild / Test / javaOptions)
- sbt-native-packager bin/<svc> launchers (per-project Universal /
javaOptions, bundled into asfLicensingSettings since the Universal
config does not propagate from ThisBuild)
- IntelliJ Application run configs (VM_PARAMETERS = @\$PROJECT_DIR\$/
.jvmopts; JDK 9+ argfile expansion at JVM start)
Also adds the missing --add-opens=java.base/java.lang.invoke flag.
Drops the partial JDK_JAVA_OPTIONS env in ComputingUnitMaster /
ComputingUnitWorker run configs (had only 3 of 6 flags; redundant
once VM_PARAMETERS points at .jvmopts).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…figs These were dropped from the committed XMLs at some point during the JDK 17 bump work. ASF source files require the header. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CheckpointSpec failed on JDK 17 because Kryo (via altoo pekko-kryo) reflects on AtomicBoolean#value when serializing WorkflowExecutionCoordinator.completionNotified. Without the open, ScalaTest reports the InaccessibleObjectException as a fast test failure rather than a hang. Single-line edit to .jvmopts; the previously committed plumbing (ThisBuild / Test / javaOptions, per-project Universal / javaOptions, .run/*.xml VM_PARAMETERS = @\$PROJECT_DIR\$/.jvmopts) propagates the new flag to every JVM the build launches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Apply agent-style passes to the comments added in this PR:
- build.sbt and project/JdkOptions.scala: drop passive voice and
column-aligned bullets; keep the why (Universal does not cascade
from ThisBuild) front-and-center.
- .jvmopts: replace one em-dash with a period.
- AGENTS.md: replace the outdated three-flag manual export snippet
with a pointer to .jvmopts as the canonical list, and to
project/JdkOptions.scala as the propagation glue. Mention the
java @.jvmopts -jar argfile form for raw-java launches.
No flag-list change; the JVM-side behavior is unchanged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The texera.amber → texera.amber.main rename is an unrelated IntelliJ sbt-import naming change that rode along with the JDK 17 fix commit. Revert to match origin/main; the rename will land in a follow-up PR covering all eight .run/*.xml files together. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…flags Without this, sbt running inside the Docker build sees no .jvmopts at the build root, so JdkOptions.jvmFlags returns Seq.empty, Universal / javaOptions ends up empty, and sbt-native-packager generates bin/<svc> launchers with no --add-opens flags. The runtime container then starts the JVM without strong-encapsulation opens, and any Pekko / Kryo / Arrow reflection throws InaccessibleObjectException. Add COPY .jvmopts .jvmopts to all eight Scala dockerfiles, right after COPY build.sbt. The runtime stage does not need it; the launcher script bakes the flags in at build time. Also rewrite the JDK 17 paragraph in AGENTS.md to align with .jvmopts: the file holds every flag Texera needs, with each group annotated by its upstream source (Kryo, Apache Arrow, Apache Pekko). Drops the Ehcache reference (its required flags are already covered by the Kryo and Pekko documentation). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
"Adding a missing flag" read as if a flag were currently missing. Reframe as future-tense: when a library upgrade or new code path triggers InaccessibleObjectException, add the open to .jvmopts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@Yicong-Huang @aglinxinyuan can you take a look at this PR again ? |
Yicong-Huang
left a comment
There was a problem hiding this comment.
I think we can merge it. There will be issues for post fixing.
What changes were proposed in this PR?
Bump all Scala/Java microservices from Java 11 to Java 17 LTS, and
centralize the JDK 17
--add-opensflags through.jvmoptsas asingle source of truth.
Toolchain bumps
bin/: build basesbtscala/scala-sbt:eclipse-temurin-jammy-11.0.17_8_1.9.3_2.13.11→
...-17.0.5_8_1.9.3_2.13.11(same sbt 1.9.3 + Scala 2.13.11);runtime
eclipse-temurin:11-{jdk,jre}-jammy→17-{jdk,jre}-jammy..github/workflows/build.ymlandbuild-and-push-images.yml:java-version: 11→17(matrix entries + 5setup-javasteps).AGENTS.md: toolchain table..jvmoptsas single source of truth.jvmoptslists the seven--add-opensflags JDK 17 needs for theruntime (Pekko, Apache Arrow, Ehcache
SizeOf, Pekko Kryoserialization). The same file reaches every JVM the build launches:
.jvmoptsdirectly.ThisBuild / Test / javaOptions ++= JdkOptions.jvmFlags(...)inbuild.sbt.bin/<svc>launchers in production dists:Universal / javaOptions ++= JdkOptions.jvmFlags(...).map("-J" + _),bundled per-project via
asfLicensingSettings(Universal does notcascade from ThisBuild).
.run/ComputingUnit{Master,Worker}.run.xml:VM_PARAMETERS=@$PROJECT_DIR$/.jvmopts(JDK 9+ argfile expansion).project/JdkOptions.scala(modeled on Pekko's) reads the file onceand exposes a
Seq[String]for the build to wire up.Any related issues, documentation, discussions?
Closes #4937. Refs discussion #4001.
How was this PR tested?
CI's amber job now finishes in
single-digit minutes, with all 5
jacocotasks passing. Localverification:
sbt show ThisBuild / Test / javaOptionsandsbt show <project> / Universal / javaOptionsconfirm flagpropagation across all subprojects;
java @.jvmopts -versionconfirms the JDK accepts the argfile cleanly; running amber
jacocolocally hits zeroInaccessibleObjectException.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.7)