[SPARK-53955][CORE][3.5] Prefer to detect Java Home from env JAVA_HOME on finding jmap for JDK 8

[pan3793]

What changes were proposed in this pull request?

Prefer to detect Java Home from env JAVA_HOME on finding jmap, then System Properties java.home

Why are the changes needed?

https://stackoverflow.com/questions/45441516/difference-between-java-home-and-java-home

JAVA_HOME points to the JDK installation path given by the Environment Variable.

But java.home points to the JRE installation path.

Somehow, it returns the same value in JDK 11+ (according to the following article, users are still able to create a JRE by themselves?), but it returns a different value in JDK 8 or prior

https://adoptium.net/news/2021/12/eclipse-temurin-jres-are-back

➜ spark-3.5.7-bin-hadoop3 bin/spark-shell
Setting default log level to "WARN".
To adjust logging level use sc.setLogLevel(newLevel). For SparkR, use setLogLevel(newLevel).
25/10/20 17:59:07 WARN NativeCodeLoader: Unable to load native-hadoop library for your platform... using builtin-java classes where applicable
Spark context Web UI available at http://HIH-D-25944Z:4040
Spark context available as 'sc' (master = local[*], app id = local-1760954347801).
Spark session available as 'spark'.
Welcome to
____ __
/ __/__ ___ _____/ /__
_\ \/ _ \/ _ `/ __/ '_/
/___/ .__/\_,_/_/ /_/\_\ version 3.5.7
/_/
Using Scala version 2.12.18 (OpenJDK 64-Bit Server VM, Java 17.0.15)
Type in expressions to have them evaluated.
Type :help for more information.
scala> System.getProperty("java.home")
res0: String = /home/chengpan/.sdkman/candidates/java/17.0.15-zulu
scala> System.getenv("JAVA_HOME")
res1: String = /home/chengpan/.sdkman/candidates/java/17.0.15-zulu

➜ spark-3.5.7-bin-hadoop3 bin/spark-shell
Setting default log level to "WARN".
To adjust logging level use sc.setLogLevel(newLevel). For SparkR, use setLogLevel(newLevel).
25/10/20 17:59:25 WARN NativeCodeLoader: Unable to load native-hadoop library for your platform... using builtin-java classes where applicable
Spark context Web UI available at http://HIH-D-25944Z:4040
Spark context available as 'sc' (master = local[*], app id = local-1760954366052).
Spark session available as 'spark'.
Welcome to
____ __
/ __/__ ___ _____/ /__
_\ \/ _ \/ _ `/ __/ '_/
/___/ .__/\_,_/_/ /_/\_\ version 3.5.7
/_/
Using Scala version 2.12.18 (OpenJDK 64-Bit Server VM, Java 1.8.0_432)
Type in expressions to have them evaluated.
Type :help for more information.
scala> System.getProperty("java.home")
res0: String = /home/chengpan/.sdkman/candidates/java/8.0.432.fx-zulu/jre
scala> System.getenv("JAVA_HOME")
res1: String = /home/chengpan/.sdkman/candidates/java/8.0.432.fx-zulu

In JDK8, jmap exists under JAVA_HOME/bin, but not under JAVA_HOME/jre/bin.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Manually verified.

Was this patch authored or co-authored using generative AI tooling?

No.

[pan3793]

cc @dongjoon-hyun as you are the author of this feature

cc @mridulm, I think the System.getProperty("java.home") comes from your suggestion, #41709 (comment). Could you please take a look at this change?

Note, this causes a real issue only on Spark 3.5 with JDK 8, but from my understanding, we should always prefer using JAVA_HOME if possible.

[sarutak]

I agree with this change as we can see similar code in SparkBuild.scala
We have also see sys.props("java.home") in AppStatusListenerSuite.scala.
Should we change it as well?

[pan3793]

@sarutak I think we should take them one by one.

AppStatusListenerSuite.scala you mentioned is actually affecting display in Spark UI, changing this may surprise users, I lean towards not touching it, but can do if someone thinks we should.

In SparkBuild.scala, I think the Spark devs already noticed that, that's why the parent dir of java.home is used, see #5441

.orElse(sys.props.get("java.home").map { p => new File(p).getParentFile().getAbsolutePath() })

it's correct for JDK 8 and prior. I think we should change it to

.orElse(sys.props.get("java.home"))

for Spark 4.0 and master, since it's not true for JDK 17+

[dongjoon-hyun]

Hi, @pan3793 .

Somehow, it returns the same value in JDK 11+ (according to the following article, users are still able to create a JRE by themselves?), but it returns a different value in JDK 8 or prior

For the above question, yes. Oracle no longer offers JRE and Server JRE downloads; consequently since Java 11.

• https://docs.oracle.com/en/java/javase/25/migrate/significant-changes-jdk-11.html

Since Java 17, openjdk Docker Image also doesn't provide *jre* tag. You can simply search it.

• https://hub.docker.com/_/openjdk/tags

Given that Apache Spark 4.0 supports Java 17+ only, I don't think this PR is valid for both master and branch-4.0. If you need this for your Java 8 environment, I'd like to recommend you to convert this to a bug issue for branch-3.5 specifically, @pan3793 . We can discuss more in that context (if exists).

[pan3793]

@dongjoon-hyun I retargeted this PR to branch-3.5

[dongjoon-hyun]

Thank you for rebasing, @pan3793 .

So, the remaining topic is the following previous comment about incompatibility. Could you revise the PR title by focusing on the case where we need to use System.getenv("JAVA_HOME") instead of System.getProperty("java.home")?

Also, there is no compatibility gaurantees that I am aware of between different versions of jdk and jmap (for example, jdk11 jmap against jdk17 or vice versa) - if I missed any, please do let me know !

[pan3793]

@dongjoon-hyun this is not about incompatibility between different versions of JDK, the real issue is, in JDK8, jmap exists under JAVA_HOME/bin, but not under JAVA_HOME/jre/bin. I updated the PR description to reflect it.

[dongjoon-hyun]

I must be clear on it. I already got your point about that but what I asked in the above is different. What I asked is more about the previous question when the different JDK versions occurs between System.getenv("JAVA_HOME") and System.getProperty("java.home").

[pan3793]

@dongjoon-hyun I see your point, it's a dedicated issue for JDK 8, I modified the PR title by adding "for JDK 8" suffix, do you think it's clear now?

[dongjoon-hyun]

Personally, I've been away from Java 8 for a long time. Since this change affects the Java 8 environment users, I believe we need other people from Databricks or LinkedIn for the final sign-off.

Anyway, +1 from my side. As the developer of this feature, I thank you for using this feature, @pan3793 .

[pan3793]

@dongjoon-hyun thanks for your review and approval

cc @HyukjinKwon@LuciferYang@viirya@mridulm could you please take a look when you have time? thank you in advance

[LuciferYang]

Merged into branch-3.5. Thanks @pan3793@dongjoon-hyun and @sarutak

[mridulm]

hmm, JAVA_HOME can point to arbitrary path and need not be valid - right ?
Given that, we should check for validity of path (more precisely if jmap is actually present at $JAVA_HOME/bin/jmap) ?

Example:

$ export JAVA_HOME=/dev/null
$ scala
Welcome to Scala 2.13.16 (OpenJDK 64-Bit Server VM, Java 17.0.17).
Type in expressions for evaluation. Or try :help.
scala> System.getenv("JAVA_HOME")
val res1: String = /dev/null
scala> System.getProperty("java.home")
val res2: String = /usr/lib/jvm/java-17-openjdk-amd64
scala> 

Thoughts @pan3793 , @LuciferYang , @dongjoon-hyun ?

[pan3793]

@mridulm Spark should correctly resolve JAVA_HOME env var or fail fast when launching by spark-submit or other shell scripts, but you are right, it's still possible that the user uses Spark as a library and creates SparkContext from their Java process.

How do you think about code below for Spark 3.5?

privatelazyvaljmapCmd:Option[String] = {
defjmapExists(maybeJavaHome: String):Boolean=Files.exists(Paths.get(maybeJavaHome, "/bin/jmap"))
Option(System.getenv("JAVA_HOME")).filter(jmapExists)
.orElse(Option(System.getProperty("java.home")).filter(jmapExists))
.map(javaHome => javaHome +"/bin/jmap")
}
/** Return a heap dump. Used to capture dumps for the web UI */defgetHeapHistogram():Array[String] = jmapCmd.map { jmap =>// From Java 9+, we can use 'ProcessHandle.current().pid()'valpid= getProcessName().split("@").head
valbuilder=newProcessBuilder(jmap, "-histo:live", pid)
valp= builder.start()
valrows=ArrayBuffer.empty[String]
Utils.tryWithResource(newBufferedReader(newInputStreamReader(p.getInputStream()))) { r =>varline=""while (line !=null) {
if (line.nonEmpty) rows += line
line = r.readLine()
}
}
rows.toArray
}.getOrElse {
logWarning("Return empty heap histogram due to jcmd is unavailable.")
Array.empty
}

Additionally, I found that jmap is still possible to be missing after Java 17, for example,

$ docker run -it ubuntu:24.04 bash
# apt update
# apt install openjdk-25-jre-headless
# ll /usr/lib/jvm/java-25-openjdk-arm64/bin/
total 280
drwxr-xr-x 2 root root 4096 Dec 8 12:30 ./
drwxr-xr-x 6 root root 4096 Dec 8 12:30 ../
-rwxr-xr-x 1 root root 67480 Oct 23 09:23 java*
-rwxr-xr-x 1 root root 67504 Oct 23 09:23 jpackage*
-rwxr-xr-x 1 root root 67504 Oct 23 09:23 keytool*
-rwxr-xr-x 1 root root 67512 Oct 23 09:23 rmiregistry*

So, maybe we also want a variant method to find jmap for the master branch?

privatelazyvaljmapCmd:Option[String] = {
defjmapExists(maybeJavaHome: String):Boolean=Files.exists(Paths.get(maybeJavaHome, "/bin/jmap"))
Option(System.getProperty("java.home")).filter(jmapExists)
.map(javaHome => javaHome +"/bin/jmap")
}