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 2021/09/09 17:29:17 UTC
[GitHub] [spark] srowen opened a new pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
srowen opened a new pull request #33947:
URL: https://github.com/apache/spark/pull/33947
### What changes were proposed in this pull request?
Improve exception handling in the Platform initialization, where it attempts to assess whether reflection is possible to modify DirectByteBuffer. This can apparently fail in more cases on Java 9+ than are currently handled, whereas Spark can continue without reflection if needed.
More detailed comments on the change inline.
### Why are the changes needed?
This exception seems to be possible and fails startup:
```
Caused by: java.lang.reflect.InaccessibleObjectException: Unable to make private java.nio.DirectByteBuffer(long,int) accessible: module java.base does not "opens java.nio" to unnamed module @71e9ddb4
at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:357)
at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
at java.base/java.lang.reflect.Constructor.checkCanSetAccessible(Constructor.java:188)
at java.base/java.lang.reflect.Constructor.setAccessible(Constructor.java:181)
at org.apache.spark.unsafe.Platform.<clinit>(Platform.java:56)
```
### Does this PR introduce _any_ user-facing change?
Should strictly allow Spark to continue in more cases.
### How was this patch tested?
Existing tests.
--
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 pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916506420
Thank you, @srowen . +1 for backporting from my side. cc @gengliangwang
> If this is OK, this should go into 3.2.x for 3.2.0 if possible
--
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] AmplabJenkins commented on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916870150
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/47649/
--
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] SparkQA commented on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916329858
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47627/
--
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] srowen commented on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-917454229
Merged to master/3.2/3.1/3.0
--
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] SparkQA commented on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916837511
**[Test build #143145 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/143145/testReport)** for PR 33947 at commit [`c30df80`](https://github.com/apache/spark/commit/c30df8093edf9c96333e1c692b29080aeb51a61f).
--
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] AmplabJenkins commented on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916951230
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/143145/
--
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] mridulm commented on a change in pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
mridulm commented on a change in pull request #33947:
URL: https://github.com/apache/spark/pull/33947#discussion_r705938781
##########
File path: common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java
##########
@@ -77,28 +67,52 @@
cleanerClassName = "jdk.internal.ref.Cleaner";
}
try {
- Class<?> cleanerClass = Class.forName(cleanerClassName);
- Method createMethod = cleanerClass.getMethod("create", Object.class, Runnable.class);
- // Accessing jdk.internal.ref.Cleaner should actually fail by default in JDK 9+,
- // unfortunately, unless the user has allowed access with something like
- // --add-opens java.base/java.lang=ALL-UNNAMED If not, we can't really use the Cleaner
- // hack below. It doesn't break, just means the user might run into the default JVM limit
- // on off-heap memory and increase it or set the flag above. This tests whether it's
- // available:
+ Class<?> cls = Class.forName("java.nio.DirectByteBuffer");
+ Constructor<?> constructor = cls.getDeclaredConstructor(Long.TYPE, Integer.TYPE);
+ Field cleanerField = cls.getDeclaredField("cleaner");
try {
- createMethod.invoke(null, null, null);
- } catch (IllegalAccessException e) {
- // Don't throw an exception, but can't log here?
- createMethod = null;
- } catch (InvocationTargetException ite) {
- // shouldn't happen; report it
- throw new IllegalStateException(ite);
+ constructor.setAccessible(true);
+ cleanerField.setAccessible(true);
+ } catch (RuntimeException re) {
+ // This is a Java 9+ exception, so needs to be handled without importing it
+ if ("InaccessibleObjectException".equals(re.getClass().getSimpleName())) {
+ // Continue, but the constructor/field are not available
+ // See comment below for more context
+ constructor = null;
+ cleanerField = null;
Review comment:
I am missing something here, given [this](https://github.com/apache/spark/blob/09d72879a9752976f11fbce6ed7be224ecb8fba0/common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java#L84), `DBB_CONSTRUCTOR` and `DBB_CLEANER_FIELD` are not set.
And as the exception is thrown from the static block, the class `Platform` will fail to load.
To illustrate:
```
$ cat Test.java
public class Test {
public static void main(String[] args) {
System.out.println("Value = " + TestStatic.value);
}
}
class TestStatic {
static final String value;
static {
if (Boolean.getBoolean("THROW_EXCEPTION")) {
throw new RuntimeException("failed");
}
value = "hi";
}
}
$ java -cp . -DTHROW_EXCEPTION=true Test
Exception in thread "main" java.lang.ExceptionInInitializerError
at Test.main(Test.java:4)
Caused by: java.lang.RuntimeException: failed
at TestStatic.<clinit>(Test.java:13)
... 1 more
```
--
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] mridulm commented on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
mridulm commented on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916600497
Rest of the changes look fine, +1 to backporting to 3.2
--
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] SparkQA commented on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916300841
**[Test build #143123 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/143123/testReport)** for PR 33947 at commit [`09d7287`](https://github.com/apache/spark/commit/09d72879a9752976f11fbce6ed7be224ecb8fba0).
--
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] srowen commented on a change in pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #33947:
URL: https://github.com/apache/spark/pull/33947#discussion_r706104756
##########
File path: common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java
##########
@@ -77,28 +67,52 @@
cleanerClassName = "jdk.internal.ref.Cleaner";
}
try {
- Class<?> cleanerClass = Class.forName(cleanerClassName);
- Method createMethod = cleanerClass.getMethod("create", Object.class, Runnable.class);
- // Accessing jdk.internal.ref.Cleaner should actually fail by default in JDK 9+,
- // unfortunately, unless the user has allowed access with something like
- // --add-opens java.base/java.lang=ALL-UNNAMED If not, we can't really use the Cleaner
- // hack below. It doesn't break, just means the user might run into the default JVM limit
- // on off-heap memory and increase it or set the flag above. This tests whether it's
- // available:
+ Class<?> cls = Class.forName("java.nio.DirectByteBuffer");
+ Constructor<?> constructor = cls.getDeclaredConstructor(Long.TYPE, Integer.TYPE);
+ Field cleanerField = cls.getDeclaredField("cleaner");
try {
- createMethod.invoke(null, null, null);
- } catch (IllegalAccessException e) {
- // Don't throw an exception, but can't log here?
- createMethod = null;
- } catch (InvocationTargetException ite) {
- // shouldn't happen; report it
- throw new IllegalStateException(ite);
+ constructor.setAccessible(true);
+ cleanerField.setAccessible(true);
+ } catch (RuntimeException re) {
+ // This is a Java 9+ exception, so needs to be handled without importing it
+ if ("InaccessibleObjectException".equals(re.getClass().getSimpleName())) {
+ // Continue, but the constructor/field are not available
+ // See comment below for more context
+ constructor = null;
+ cleanerField = null;
Review comment:
Ugh, yeah I made a basic error here - needs to be 'else throw re'.
I should also say I don't know how this arises, so can't test for it. It came up on the list and seems therefore possible, but also didn't come up before in testing
--
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] AmplabJenkins removed a comment on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916415876
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/143123/
--
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] SparkQA commented on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916860391
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47649/
--
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] SparkQA commented on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916400163
**[Test build #143123 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/143123/testReport)** for PR 33947 at commit [`09d7287`](https://github.com/apache/spark/commit/09d72879a9752976f11fbce6ed7be224ecb8fba0).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
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] AmplabJenkins removed a comment on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916951230
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/143145/
--
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] AmplabJenkins commented on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916339477
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/47627/
--
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] srowen closed pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
srowen closed pull request #33947:
URL: https://github.com/apache/spark/pull/33947
--
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] srowen commented on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916297150
If this is OK, this should go into 3.2.x for 3.2.0 if possible
--
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] gengliangwang commented on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916663526
+1 to backporting to 3.2, too
--
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] SparkQA commented on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916933770
**[Test build #143145 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/143145/testReport)** for PR 33947 at commit [`c30df80`](https://github.com/apache/spark/commit/c30df8093edf9c96333e1c692b29080aeb51a61f).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
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] SparkQA commented on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916336595
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47627/
--
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] AmplabJenkins commented on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916415876
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/143123/
--
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] AmplabJenkins removed a comment on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916339477
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/47627/
--
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] mridulm commented on a change in pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
mridulm commented on a change in pull request #33947:
URL: https://github.com/apache/spark/pull/33947#discussion_r705866414
##########
File path: common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java
##########
@@ -77,28 +67,52 @@
cleanerClassName = "jdk.internal.ref.Cleaner";
}
try {
- Class<?> cleanerClass = Class.forName(cleanerClassName);
- Method createMethod = cleanerClass.getMethod("create", Object.class, Runnable.class);
- // Accessing jdk.internal.ref.Cleaner should actually fail by default in JDK 9+,
- // unfortunately, unless the user has allowed access with something like
- // --add-opens java.base/java.lang=ALL-UNNAMED If not, we can't really use the Cleaner
- // hack below. It doesn't break, just means the user might run into the default JVM limit
- // on off-heap memory and increase it or set the flag above. This tests whether it's
- // available:
+ Class<?> cls = Class.forName("java.nio.DirectByteBuffer");
+ Constructor<?> constructor = cls.getDeclaredConstructor(Long.TYPE, Integer.TYPE);
+ Field cleanerField = cls.getDeclaredField("cleaner");
try {
- createMethod.invoke(null, null, null);
- } catch (IllegalAccessException e) {
- // Don't throw an exception, but can't log here?
- createMethod = null;
- } catch (InvocationTargetException ite) {
- // shouldn't happen; report it
- throw new IllegalStateException(ite);
+ constructor.setAccessible(true);
+ cleanerField.setAccessible(true);
+ } catch (RuntimeException re) {
+ // This is a Java 9+ exception, so needs to be handled without importing it
+ if ("InaccessibleObjectException".equals(re.getClass().getSimpleName())) {
+ // Continue, but the constructor/field are not available
+ // See comment below for more context
+ constructor = null;
+ cleanerField = null;
Review comment:
Given the `throw`, why do we need this ? Class wont be initialized right ?
--
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] SparkQA removed a comment on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916837511
**[Test build #143145 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/143145/testReport)** for PR 33947 at commit [`c30df80`](https://github.com/apache/spark/commit/c30df8093edf9c96333e1c692b29080aeb51a61f).
--
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] SparkQA commented on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916865380
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47649/
--
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] SparkQA removed a comment on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916300841
**[Test build #143123 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/143123/testReport)** for PR 33947 at commit [`09d7287`](https://github.com/apache/spark/commit/09d72879a9752976f11fbce6ed7be224ecb8fba0).
--
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] srowen commented on a change in pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #33947:
URL: https://github.com/apache/spark/pull/33947#discussion_r705553639
##########
File path: common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java
##########
@@ -77,28 +67,52 @@
cleanerClassName = "jdk.internal.ref.Cleaner";
}
try {
- Class<?> cleanerClass = Class.forName(cleanerClassName);
- Method createMethod = cleanerClass.getMethod("create", Object.class, Runnable.class);
- // Accessing jdk.internal.ref.Cleaner should actually fail by default in JDK 9+,
- // unfortunately, unless the user has allowed access with something like
- // --add-opens java.base/java.lang=ALL-UNNAMED If not, we can't really use the Cleaner
- // hack below. It doesn't break, just means the user might run into the default JVM limit
- // on off-heap memory and increase it or set the flag above. This tests whether it's
- // available:
+ Class<?> cls = Class.forName("java.nio.DirectByteBuffer");
Review comment:
This was simply moved from above to further unify handling of exceptions during init
##########
File path: common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java
##########
@@ -77,28 +67,52 @@
cleanerClassName = "jdk.internal.ref.Cleaner";
}
try {
- Class<?> cleanerClass = Class.forName(cleanerClassName);
- Method createMethod = cleanerClass.getMethod("create", Object.class, Runnable.class);
- // Accessing jdk.internal.ref.Cleaner should actually fail by default in JDK 9+,
- // unfortunately, unless the user has allowed access with something like
- // --add-opens java.base/java.lang=ALL-UNNAMED If not, we can't really use the Cleaner
- // hack below. It doesn't break, just means the user might run into the default JVM limit
- // on off-heap memory and increase it or set the flag above. This tests whether it's
- // available:
+ Class<?> cls = Class.forName("java.nio.DirectByteBuffer");
+ Constructor<?> constructor = cls.getDeclaredConstructor(Long.TYPE, Integer.TYPE);
+ Field cleanerField = cls.getDeclaredField("cleaner");
try {
- createMethod.invoke(null, null, null);
- } catch (IllegalAccessException e) {
- // Don't throw an exception, but can't log here?
- createMethod = null;
- } catch (InvocationTargetException ite) {
- // shouldn't happen; report it
- throw new IllegalStateException(ite);
+ constructor.setAccessible(true);
+ cleanerField.setAccessible(true);
+ } catch (RuntimeException re) {
+ // This is a Java 9+ exception, so needs to be handled without importing it
+ if ("InaccessibleObjectException".equals(re.getClass().getSimpleName())) {
Review comment:
This is really the new exception handling; now have to catch this and also handle the case where DBB_CONSTRUCTOR and DBB_CLEANER_FIELD aren't available.
--
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] srowen commented on a change in pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #33947:
URL: https://github.com/apache/spark/pull/33947#discussion_r705881771
##########
File path: common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java
##########
@@ -77,28 +67,52 @@
cleanerClassName = "jdk.internal.ref.Cleaner";
}
try {
- Class<?> cleanerClass = Class.forName(cleanerClassName);
- Method createMethod = cleanerClass.getMethod("create", Object.class, Runnable.class);
- // Accessing jdk.internal.ref.Cleaner should actually fail by default in JDK 9+,
- // unfortunately, unless the user has allowed access with something like
- // --add-opens java.base/java.lang=ALL-UNNAMED If not, we can't really use the Cleaner
- // hack below. It doesn't break, just means the user might run into the default JVM limit
- // on off-heap memory and increase it or set the flag above. This tests whether it's
- // available:
+ Class<?> cls = Class.forName("java.nio.DirectByteBuffer");
+ Constructor<?> constructor = cls.getDeclaredConstructor(Long.TYPE, Integer.TYPE);
+ Field cleanerField = cls.getDeclaredField("cleaner");
try {
- createMethod.invoke(null, null, null);
- } catch (IllegalAccessException e) {
- // Don't throw an exception, but can't log here?
- createMethod = null;
- } catch (InvocationTargetException ite) {
- // shouldn't happen; report it
- throw new IllegalStateException(ite);
+ constructor.setAccessible(true);
+ cleanerField.setAccessible(true);
+ } catch (RuntimeException re) {
+ // This is a Java 9+ exception, so needs to be handled without importing it
+ if ("InaccessibleObjectException".equals(re.getClass().getSimpleName())) {
+ // Continue, but the constructor/field are not available
+ // See comment below for more context
+ constructor = null;
+ cleanerField = null;
Review comment:
This block helps ensure the class does initialize even if reflection can't be used on DirectByteBuffer. Nulling these particular values out doesn't matter a whole lot; without it, the two static final variables do get set to values that can't be used anyway. It seemed tidier and more symmetric to just ensure they init to null in this case, though later they won't even be used if the rest didn't succeed in setting up values necessary to make reflection work.
--
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] AmplabJenkins removed a comment on pull request #33947: [SPARK-36704][CORE] Expand exception handling to more Java 9 cases where reflection is limited at runtime, when reflecting to manage DirectByteBuffer settings
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33947:
URL: https://github.com/apache/spark/pull/33947#issuecomment-916870150
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/47649/
--
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