You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/10/10 12:40:52 UTC

[GitHub] [spark] LuciferYang opened a new pull request, #38190: [SPARK-40729][BUILD][SHELL] Add `-Djdk.reflect.useDirectMethodHandle=false` to spark-shell and `maven/sbt` test options

LuciferYang opened a new pull request, #38190:
URL: https://github.com/apache/spark/pull/38190

   ### What changes were proposed in this pull request?
   This pr add `-Djdk.reflect.useDirectMethodHandle=false` to spark-shell and `maven/sbt` test options to make:
   
   1. repl module test passed with Java 19
   2. spark-shell can run the bad case described in SPARK-40729 with Java 19
   
   
   ### Why are the changes needed?
   `MethodHandleAccessor` is used by default instead of `UnsafeFieldAccessor` after [JEP 416: Reimplement Core Reflection with Method Handles](https://openjdk.org/jeps/416), enable the old implementation via `-Djdk.reflect.useDirectMethodHandle=false` as a workaround.
   
   
   ### Does this PR introduce _any_ user-facing change?
   No
   
   
   ### How was this patch tested?
   
   - Pass GitHub Actions
   


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on pull request #38190: [WIP][SPARK-40729][BUILD][SHELL] Use `-Djdk.reflect.useDirectMethodHandle=false` to make Java18/19 use `UnsafeFieldAccessor` instead of `MethodHandleAccessor`

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #38190:
URL: https://github.com/apache/spark/pull/38190#issuecomment-1273352918

   close this due to sbt has other issue


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38190: [SPARK-40729][CORE] Make Spark use `UnsafeFieldAccessor` as default with Java18/19

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38190:
URL: https://github.com/apache/spark/pull/38190#discussion_r993037177


##########
launcher/src/main/java/org/apache/spark/launcher/JavaModuleOptions.java:
##########
@@ -40,7 +40,8 @@ public class JavaModuleOptions {
       "--add-opens=java.base/sun.nio.cs=ALL-UNNAMED",
       "--add-opens=java.base/sun.security.action=ALL-UNNAMED",
       "--add-opens=java.base/sun.util.calendar=ALL-UNNAMED",
-      "--add-opens=java.security.jgss/sun.security.krb5=ALL-UNNAMED"};
+      "--add-opens=java.security.jgss/sun.security.krb5=ALL-UNNAMED",
+      "-Djdk.reflect.useDirectMethodHandle=false"};

Review Comment:
   Ya, I agree with you. Let's postpone the renaming.



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang closed pull request #38190: [WIP][SPARK-40729][BUILD][SHELL] Use `-Djdk.reflect.useDirectMethodHandle=false` to make Java18/19 use `UnsafeFieldAccessor` instead of `MethodHandleAccessor`

Posted by GitBox <gi...@apache.org>.
LuciferYang closed pull request #38190: [WIP][SPARK-40729][BUILD][SHELL] Use `-Djdk.reflect.useDirectMethodHandle=false` to make Java18/19 use `UnsafeFieldAccessor` instead of `MethodHandleAccessor` 
URL: https://github.com/apache/spark/pull/38190


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on a diff in pull request #38190: [WIP][SPARK-40729][CORE] Make Spark use `UnsafeFieldAccessor` as default with Java18/19

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38190:
URL: https://github.com/apache/spark/pull/38190#discussion_r992956201


##########
launcher/src/main/java/org/apache/spark/launcher/JavaModuleOptions.java:
##########
@@ -40,7 +40,8 @@ public class JavaModuleOptions {
       "--add-opens=java.base/sun.nio.cs=ALL-UNNAMED",
       "--add-opens=java.base/sun.security.action=ALL-UNNAMED",
       "--add-opens=java.base/sun.util.calendar=ALL-UNNAMED",
-      "--add-opens=java.security.jgss/sun.security.krb5=ALL-UNNAMED"};
+      "--add-opens=java.security.jgss/sun.security.krb5=ALL-UNNAMED",
+      "-Djdk.reflect.useDirectMethodHandle=false"};

Review Comment:
   It is better to rename `JavaModuleOptions` as `JavaExtraOptions`. Do you suggest completing the corresponding refactor work in this pr? @dongjoon-hyun 



##########
launcher/src/main/java/org/apache/spark/launcher/JavaModuleOptions.java:
##########
@@ -40,7 +40,8 @@ public class JavaModuleOptions {
       "--add-opens=java.base/sun.nio.cs=ALL-UNNAMED",
       "--add-opens=java.base/sun.security.action=ALL-UNNAMED",
       "--add-opens=java.base/sun.util.calendar=ALL-UNNAMED",
-      "--add-opens=java.security.jgss/sun.security.krb5=ALL-UNNAMED"};
+      "--add-opens=java.security.jgss/sun.security.krb5=ALL-UNNAMED",
+      "-Djdk.reflect.useDirectMethodHandle=false"};

Review Comment:
   I think it is better to rename `JavaModuleOptions` as `JavaExtraOptions`. Do you suggest completing the corresponding refactor work in this pr? @dongjoon-hyun 



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on pull request #38190: [WIP][SPARK-40729][BUILD][SHELL] Use `-Djdk.reflect.useDirectMethodHandle=false` to make Java18/19 use `UnsafeFieldAccessor` in Spark-Shell

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #38190:
URL: https://github.com/apache/spark/pull/38190#issuecomment-1274383707

   cc @rednaxelafx 


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] rednaxelafx commented on a diff in pull request #38190: [WIP][SPARK-40729][BUILD][SHELL] Use `-Djdk.reflect.useDirectMethodHandle=false` to make Java18/19 use `UnsafeFieldAccessor` in Spark-Shell

Posted by GitBox <gi...@apache.org>.
rednaxelafx commented on code in PR #38190:
URL: https://github.com/apache/spark/pull/38190#discussion_r992864456


##########
pom.xml:
##########
@@ -3011,7 +3011,8 @@
             <reportsDirectory>${project.build.directory}/surefire-reports</reportsDirectory>
             <junitxml>.</junitxml>
             <filereports>SparkTestSuite.txt</filereports>
-            <argLine>-ea -Xmx4g -Xss4m -XX:MaxMetaspaceSize=2g -XX:ReservedCodeCacheSize=${CodeCacheSize} ${extraJavaTestArgs} -Dio.netty.tryReflectionSetAccessible=true</argLine>
+            <argLine>-ea -Xmx4g -Xss4m -XX:MaxMetaspaceSize=2g -XX:ReservedCodeCacheSize=${CodeCacheSize} ${extraJavaTestArgs} -Dio.netty.tryReflectionSetAccessible=true
+              -Djdk.reflect.useDirectMethodHandle=false</argLine>

Review Comment:
   The issue seem to be that the ClosureCleaner is trying to set a final field which shouldn't be allowed. This is Spark-specific (and affects all other projects that copied Spark's ClosureCleaner), and doesn't exist in the vanilla Scala REPL.



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] dongjoon-hyun closed pull request #38190: [SPARK-40729][CORE] Make Spark use `UnsafeFieldAccessor` as default for Java 18+

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #38190: [SPARK-40729][CORE] Make Spark use `UnsafeFieldAccessor` as default for Java 18+
URL: https://github.com/apache/spark/pull/38190


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on pull request #38190: [WIP][SPARK-40729][BUILD][SHELL] Use `-Djdk.reflect.useDirectMethodHandle=false` to make Java18/19 use `UnsafeFieldAccessor` instead of `MethodHandleAccessor`

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #38190:
URL: https://github.com/apache/spark/pull/38190#issuecomment-1274372758

   https://github.com/apache/spark/blob/8e853933ba64bb43e02af78198bc36d471efaa18/core/src/main/scala/org/apache/spark/util/ClosureCleaner.scala#L397-L408
   
   `outerField.set(func, clonedOuterThis)` threw ` java.lang.IllegalAccessException: final field has no write access: $Lambda$3061/0x0000000801e01000.arg$1/putField, from class java.lang.Object (module java.base)`, although we have deleted `FINAL Modifier`.
   
   Any suggestions for code level fix? `-Djdk.reflect.useDirectMethodHandle=false` will be also deleted in the future
   
   
   
   
   


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on pull request #38190: [WIP][SPARK-40729][BUILD][SHELL] Use `-Djdk.reflect.useDirectMethodHandle=false` to make Java18/19 use `UnsafeFieldAccessor` in Spark-Shell

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #38190:
URL: https://github.com/apache/spark/pull/38190#issuecomment-1275525336

   > If this is required, why don't we put this into `JavaModuleOptions` like the other Java options?
   
   Yes, this is a feasible way, let me try it.


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on a diff in pull request #38190: [WIP][SPARK-40729][CORE] Make Spark use `UnsafeFieldAccessor` as default with Java18/19

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38190:
URL: https://github.com/apache/spark/pull/38190#discussion_r992964089


##########
launcher/src/main/java/org/apache/spark/launcher/JavaModuleOptions.java:
##########
@@ -40,7 +40,8 @@ public class JavaModuleOptions {
       "--add-opens=java.base/sun.nio.cs=ALL-UNNAMED",
       "--add-opens=java.base/sun.security.action=ALL-UNNAMED",
       "--add-opens=java.base/sun.util.calendar=ALL-UNNAMED",
-      "--add-opens=java.security.jgss/sun.security.krb5=ALL-UNNAMED"};
+      "--add-opens=java.security.jgss/sun.security.krb5=ALL-UNNAMED",
+      "-Djdk.reflect.useDirectMethodHandle=false"};

Review Comment:
   My idea is that this pr solves the issue first if need, and then do the rename work by a separate pr



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on a diff in pull request #38190: [SPARK-40729][CORE] Make Spark use `UnsafeFieldAccessor` as default with Java18/19

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38190:
URL: https://github.com/apache/spark/pull/38190#discussion_r993010384


##########
pom.xml:
##########
@@ -3011,7 +3011,8 @@
             <reportsDirectory>${project.build.directory}/surefire-reports</reportsDirectory>
             <junitxml>.</junitxml>
             <filereports>SparkTestSuite.txt</filereports>
-            <argLine>-ea -Xmx4g -Xss4m -XX:MaxMetaspaceSize=2g -XX:ReservedCodeCacheSize=${CodeCacheSize} ${extraJavaTestArgs} -Dio.netty.tryReflectionSetAccessible=true</argLine>
+            <argLine>-ea -Xmx4g -Xss4m -XX:MaxMetaspaceSize=2g -XX:ReservedCodeCacheSize=${CodeCacheSize} ${extraJavaTestArgs} -Dio.netty.tryReflectionSetAccessible=true
+              -Djdk.reflect.useDirectMethodHandle=false</argLine>

Review Comment:
   Yes, only Apache Spark REPL issue?



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38190: [SPARK-40729][CORE] Make Spark use `UnsafeFieldAccessor` as default with Java18/19

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38190:
URL: https://github.com/apache/spark/pull/38190#discussion_r993036210


##########
pom.xml:
##########
@@ -3011,7 +3011,8 @@
             <reportsDirectory>${project.build.directory}/surefire-reports</reportsDirectory>
             <junitxml>.</junitxml>
             <filereports>SparkTestSuite.txt</filereports>
-            <argLine>-ea -Xmx4g -Xss4m -XX:MaxMetaspaceSize=2g -XX:ReservedCodeCacheSize=${CodeCacheSize} ${extraJavaTestArgs} -Dio.netty.tryReflectionSetAccessible=true</argLine>
+            <argLine>-ea -Xmx4g -Xss4m -XX:MaxMetaspaceSize=2g -XX:ReservedCodeCacheSize=${CodeCacheSize} ${extraJavaTestArgs} -Dio.netty.tryReflectionSetAccessible=true
+              -Djdk.reflect.useDirectMethodHandle=false</argLine>

Review Comment:
   Got it. Thank you, @rednaxelafx and @LuciferYang .



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38190: [WIP][SPARK-40729][BUILD][SHELL] Use `-Djdk.reflect.useDirectMethodHandle=false` to make Java18/19 use `UnsafeFieldAccessor` in Spark-Shell

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38190:
URL: https://github.com/apache/spark/pull/38190#discussion_r992779327


##########
pom.xml:
##########
@@ -3011,7 +3011,8 @@
             <reportsDirectory>${project.build.directory}/surefire-reports</reportsDirectory>
             <junitxml>.</junitxml>
             <filereports>SparkTestSuite.txt</filereports>
-            <argLine>-ea -Xmx4g -Xss4m -XX:MaxMetaspaceSize=2g -XX:ReservedCodeCacheSize=${CodeCacheSize} ${extraJavaTestArgs} -Dio.netty.tryReflectionSetAccessible=true</argLine>
+            <argLine>-ea -Xmx4g -Xss4m -XX:MaxMetaspaceSize=2g -XX:ReservedCodeCacheSize=${CodeCacheSize} ${extraJavaTestArgs} -Dio.netty.tryReflectionSetAccessible=true
+              -Djdk.reflect.useDirectMethodHandle=false</argLine>

Review Comment:
   What about the vanilla Scala REPL? Is this only Apache Spark REPL issue? 



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on a diff in pull request #38190: [SPARK-40729][CORE] Make Spark use `UnsafeFieldAccessor` as default with Java18/19

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38190:
URL: https://github.com/apache/spark/pull/38190#discussion_r993038614


##########
launcher/src/main/java/org/apache/spark/launcher/JavaModuleOptions.java:
##########
@@ -40,7 +40,8 @@ public class JavaModuleOptions {
       "--add-opens=java.base/sun.nio.cs=ALL-UNNAMED",
       "--add-opens=java.base/sun.security.action=ALL-UNNAMED",
       "--add-opens=java.base/sun.util.calendar=ALL-UNNAMED",
-      "--add-opens=java.security.jgss/sun.security.krb5=ALL-UNNAMED"};
+      "--add-opens=java.security.jgss/sun.security.krb5=ALL-UNNAMED",
+      "-Djdk.reflect.useDirectMethodHandle=false"};

Review Comment:
   Thanks @dongjoon-hyun 



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on pull request #38190: [SPARK-40729][CORE] Make Spark use `UnsafeFieldAccessor` as default for Java 18+

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #38190:
URL: https://github.com/apache/spark/pull/38190#issuecomment-1275661593

   thanks @dongjoon-hyun @rednaxelafx 


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org