[fix](regression) use JDK 17 for Java UDF builds#65888
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review summary
I reviewed both changed files and found one blocking portability issue, submitted inline: the new hard JDK 17 check retains the carriage return in Windows/Cygwin CRLF property output and therefore rejects a valid JDK 17 before any script mode can run.
Critical checkpoint conclusions:
- Goal and test proof: The change otherwise centralizes JDK 17 selection before Maven and builds the Java-UDF fixture for release 17. The PR reports successful compile and P0, non-concurrent, external, cloud, and vault regression checks, and the author verified class-file version 61. Local
bash -nand ShellCheck also pass. Those checks do not cover the reproduced CRLF parser failure, so the goal is incomplete on Cygwin. - Scope and focus: The two-file change is small and focused on regression-test Java selection and bytecode target.
- Concurrency and lifecycle: No concurrent code, shared mutable state, static initialization, or special Doris object lifecycle is introduced. The environment mutation is process-local, and the existing framework/UDF build-and-copy lifecycle remains coherent.
- Configuration and compatibility: No Doris config, FE-BE protocol, storage format, rolling-upgrade state, or persisted metadata changes. Current master FE/BE launchers require exactly JDK 17, so class version 61 is compatible with supported master runtime paths; the older-JVM suspicion was dismissed with code evidence. The remaining compatibility defect is the Cygwin CRLF parse.
- Parallel and conditional paths: Explicit
JAVA_HOME/JDK_17, macOS discovery, Linux discovery, dynamic-file UDF loading, andcustom_libloading were checked. Parallel Doris JDK helpers already normalize carriage returns for Cygwin, but this new parser does not. - Tests and results: No regression
.outfiles change. Existing PR checks cover normal Linux/runtime behavior but not CRLF input. I reproduced the parser returning bytes31 37 0dfor a valid17property line. Per the review-only instructions, I did not build Doris or regenerate artifacts locally. - Observability and performance: The selected JDK and failures are logged; no new metric is needed. Discovery is startup-only and has no material hot-path cost.
- Transactions, persistence, and data writes: Not applicable; no transaction, visibility, edit-log, or data-write path is touched.
- Other concerns: No additional substantiated correctness, error-handling, resource, or coverage issue remains after deduplication.
Subagent convergence: In round 3, both normal full-review agents and the separate risk-focused agent returned NO_NEW_VALUABLE_FINDINGS for this exact one-comment set.
User focus: No additional focus was provided.
Review status: Complete with one requested change.
| local java_cmd="$1" | ||
| local spec_version | ||
|
|
||
| spec_version="$("${java_cmd}" -XshowSettings:properties -version 2>&1 \ |
There was a problem hiding this comment.
[P1] Normalize CRLF before the hard JDK check
On Cygwin, Java's CRLF settings output makes this parser return a value ending in \r. java_major_version passes that through, so is_jdk17_home rejects even a valid explicit JDK 17 and the script exits before Maven or argument handling. The existing JDK helpers in env.sh, start_be.sh, start_fe.sh, run-be-ut.sh, and tools/find_libjvm.sh already normalize \r specifically for Cygwin. Please strip the carriage return before comparing the major version, and cover CRLF input in the parser test.
|
/review |
There was a problem hiding this comment.
Automated follow-up review summary
The current head fixes the previously reported Cygwin/CRLF defect by stripping \r before comparing the Java specification version. I found no new blocking issue. The existing thread also requested parser regression coverage; no such test was added, so that coverage request remains existing review context, but I did not duplicate it inline.
Critical checkpoint conclusions:
- Goal and test proof: The change consistently selects an exact JDK 17 before Maven, builds both the framework and Java-UDF fixture with it, and emits the fixture with
release=17. I independently ranbash -n, ShellCheck, diff whitespace checking, and function-level checks for explicit JDK 17, newer/invalidJAVA_HOMEplusJDK_17, Linux fallback, and CRLF normalization. Per the review-only instructions, I did not build Doris or regenerate artifacts. The PR body reports a successful clean selection check and Java-UDF build/class-version inspection; those are author-reported, not rerun here. - Scope and focus: The two-file change is small and focused on regression-test JDK ownership and the UDF fixture bytecode target.
- Concurrency and lifecycle: No concurrent code, shared mutable state, static initialization, or special Doris object lifecycle is introduced. Environment changes are process-local, and the framework/UDF build-copy-run sequence remains coherent.
- Configuration and compatibility: No Doris configuration, FE-BE protocol, storage format, rolling-upgrade state, or persisted metadata changes. Maven Compiler Plugin 3.10.1 supports
release=17; current FE and BE launchers require exactly JDK 17, and no supported current-tree path was found that automatically loads this generated jar on an older JVM. - Parallel and conditional paths: Explicit
JAVA_HOME,JDK_17, macOS discovery, Linux discovery, Cygwin parsing, Maven handoff, direct jar loading,custom_lib, and clean/compile/run modes were checked. The clean-only concern was dismissed because the complete path already performs the unconditional Maven/Java preflight, and exact JDK-17 validation before Maven is the stated contract. - Tests and results: No regression
.outfile changes. Existing Java-UDF suites exercise the artifact load paths, but the parser-specific regression test requested in the existing thread is still absent. - Observability and performance: JDK selection and failure are logged. Discovery is startup-only, with no material hot-path cost; no new metric is needed.
- Transactions, persistence, and data writes: Not applicable.
- Other concerns: A related pipeline comment still claims the fixture must stay Java 8, but current FE/BE launcher code proves that wording is stale; it has no runtime effect on this patch.
Subagent convergence: In round 2, both normal full-review agents and the separate risk-focused agent returned NO_NEW_VALUABLE_FINDINGS for this exact empty inline set after the sole new candidate was independently dismissed with complete-control-flow evidence.
User focus: No additional focus was provided.
Review status: Complete. No new inline comments; no additional blocking issue found on the current head.
|
流水线已跑过,失败的和本PR无关,skip了 |
|
skip buildall |
|
PR approved by at least one committer and no changes requested. |
## What changed - make `run-regression-test.sh` select and validate JDK 17 before invoking Maven - prefer a valid `JAVA_HOME` or `JDK_17`, with Linux and macOS discovery fallbacks - build the regression framework and Java UDF case jar with the same JDK 17 - compile `java-udf-src` with `maven.compiler.release=17` ## Why The regression build depended on the caller to prepare `JAVA_HOME`, and newer branches also switched to JDK 8 only for the Java UDF module. This split ownership between CI and the Doris build script and could produce incomplete case artifacts when the framework required JDK 17. Keeping Java selection in `run-regression-test.sh` makes local and CI builds follow the same contract and removes the Java 8-only UDF build path. ## Validation - `bash -n run-regression-test.sh` - `git diff --check` - ran `./run-regression-test.sh --clean` while the incoming `JAVA_HOME` pointed to JDK 22; the script selected JDK 17 and Maven reported Java 17 - built `regression-test/java-udf-src` with JDK 17 successfully - verified `Echo$EchoInt` has class major version 61 and the expected UDF classes are present in the assembled jar
What changed
run-regression-test.shselect and validate JDK 17 before invoking MavenJAVA_HOMEorJDK_17, with Linux and macOS discovery fallbacksjava-udf-srcwithmaven.compiler.release=17Why
The regression build depended on the caller to prepare
JAVA_HOME, and newer branches also switched to JDK 8 only for the Java UDF module. This split ownership between CI and the Doris build script and could produce incomplete case artifacts when the framework required JDK 17.Keeping Java selection in
run-regression-test.shmakes local and CI builds follow the same contract and removes the Java 8-only UDF build path.Validation
bash -n run-regression-test.shgit diff --check./run-regression-test.sh --cleanwhile the incomingJAVA_HOMEpointed to JDK 22; the script selected JDK 17 and Maven reported Java 17regression-test/java-udf-srcwith JDK 17 successfullyEcho$EchoInthas class major version 61 and the expected UDF classes are present in the assembled jar