Copilot commented on code in PR #12543:
URL: https://github.com/apache/gluten/pull/12543#discussion_r3600767170
##########
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:
Switching from reflection to a direct reference changes the failure mode
when spark-launcher (or JavaModuleOptions) is missing from the runtime
classpath: this will now fail during class/method resolution with a
NoClassDefFoundError, losing the prior targeted RuntimeException message.
Consider keeping a small guard to rethrow with a clearer diagnostic while still
using the direct call.
##########
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:
Grammar/capitalization in this updated comment reads awkwardly ("we do
cleanup spark..."). Consider rephrasing to "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]
