LuciferYang commented on code in PR #12543:
URL: https://github.com/apache/gluten/pull/12543#discussion_r3600980989
##########
tools/gluten-it/common/src/main/java/org/apache/gluten/integration/SparkJvmOptions.java:
##########
@@ -16,28 +16,12 @@
*/
package org.apache.gluten.integration;
-import java.lang.reflect.InvocationTargetException;
-import java.lang.reflect.Method;
+import org.apache.spark.launcher.JavaModuleOptions;
public class SparkJvmOptions {
- private static final String MODULE_OPTIONS_CLASS_NAME =
- "org.apache.spark.launcher.JavaModuleOptions";
public static String read() {
- try {
- final Class<?> clazz = Class.forName(MODULE_OPTIONS_CLASS_NAME);
- final Method method = clazz.getMethod("defaultModuleOptions");
- return (String) method.invoke(null);
- } catch (ClassNotFoundException
- | NoSuchMethodException
- | InvocationTargetException
- | IllegalAccessException e) {
- throw new RuntimeException(
- "Failed to read Spark JVM module options via "
- + MODULE_OPTIONS_CLASS_NAME
- + "#defaultModuleOptions",
- e);
- }
+ return JavaModuleOptions.defaultModuleOptions();
}
Review Comment:
Thanks for the note. I don't think a guard is worth adding here:
- `spark-launcher` is a `compile`-scope dep of `spark-core` on every
currently supported branch (Spark 3.3 through 4.1), so `JavaModuleOptions` will
always be on the classpath at runtime.
- The only caller is `tools/gluten-it/sbin/gluten-it.sh`, which runs
`SparkJvmOptions` against the `package/target/lib` directory containing the
full Gluten + Spark jars — the same classpath every Gluten job uses.
A guard would trade a standard JVM linkage error for a slightly nicer
message in a scenario that has no realistic way to occur; every other Gluten
module already calls Spark APIs directly and would fail earlier under a
hypothetical missing-`spark-core` setup. I'd rather match that convention.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/WriteFilesExecTransformer.scala:
##########
@@ -287,7 +287,8 @@ object WriteFilesExecTransformer {
.toLowerCase(Locale.ROOT)
}
- // To be compatible with Spark3.2/3.3/3.4, we do cleanup spark internal
metadata manually.
+ // To be compatible with Spark 3.3/3.4, we do cleanup spark internal
metadata manually.
Review Comment:
Good catch, fixed in bf250e982 — "we do cleanup spark..." → "we clean up
Spark...".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
