You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "JoshRosen (via GitHub)" <gi...@apache.org> on 2023/10/12 01:50:05 UTC
[PR] [SPARK-45508][CORE] Add "--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED" so Platform can access cleaner on Java 9+ [spark]
JoshRosen opened a new pull request, #43344:
URL: https://github.com/apache/spark/pull/43344
### What changes were proposed in this pull request?
This PR adds `--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED` to our JVM flags so that we can access `jdk.internal.ref.Cleaner` in JDK 9+.
### Why are the changes needed?
This allows Spark to allocate direct memory while ignoring the JVM's MaxDirectMemorySize limit. Spark uses JDK internal APIs to directly construct DirectByteBuffers while bypassing that limit, but there is a fallback path at https://github.com/apache/spark/blob/v3.5.0/common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java#L213 that is used if we cannot reflectively access the `Cleaner` API.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added a unit test in `PlatformUtilSuite`.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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
Re: [PR] [SPARK-45508][CORE] Add "--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED" so Platform can access Cleaner on Java 9+ [spark]
Posted by "JoshRosen (via GitHub)" <gi...@apache.org>.
JoshRosen commented on PR #43344:
URL: https://github.com/apache/spark/pull/43344#issuecomment-1761931294
> I backported to branch-3.5/3.4/3.5 manually updating from `Assertions.` to `Assert.`.
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
Re: [PR] [SPARK-45508][CORE] Add "--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED" so Platform can access Cleaner on Java 9+ [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43344:
URL: https://github.com/apache/spark/pull/43344#discussion_r1356005221
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/PlatformUtilSuite.java:
##########
@@ -157,4 +157,11 @@ public void heapMemoryReuse() {
Assertions.assertEquals(1024 * 1024 + 7, onheap4.size());
Assertions.assertEquals(obj3, onheap4.getBaseObject());
}
+
+ @Test
+ public void cleanerCreateMethodIsDefined() {
+ // Regression test for SPARK-45508: we don't expect the "no cleaner" fallback
+ // path to be hit in normal usage.
+ Assertions.assertTrue(Platform.cleanerCreateMethodIsDefined());
Review Comment:
> If we want to backport this change then we will have to prepare a separate PR because #43074 changed the JUnit assertions helper names so the same source can't compile in all branches.
sorry for making this change ...
--
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
Re: [PR] [SPARK-45508][CORE] Add "--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED" so Platform can access Cleaner on Java 9+ [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang closed pull request #43344: [SPARK-45508][CORE] Add "--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED" so Platform can access Cleaner on Java 9+
URL: https://github.com/apache/spark/pull/43344
--
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
Re: [PR] [SPARK-45508][CORE] Add "--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED" so Platform can access Cleaner on Java 9+ [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #43344:
URL: https://github.com/apache/spark/pull/43344#issuecomment-1761819209
I backported to branch-3.5/3.4/3.5 manually updating from `Assertions.` to `Assert.`.
```
$ build/sbt "unsafe/testOnly *.PlatformUtilSuite"
...
[info] Test run org.apache.spark.unsafe.PlatformUtilSuite started
[info] Test org.apache.spark.unsafe.PlatformUtilSuite.freeingOnHeapMemoryBlockResetsBaseObjectAndOffset started
[info] Test org.apache.spark.unsafe.PlatformUtilSuite.overlappingCopyMemory started
[info] Test org.apache.spark.unsafe.PlatformUtilSuite.memoryDebugFillEnabledInTest started
[info] Test org.apache.spark.unsafe.PlatformUtilSuite.offHeapMemoryAllocatorThrowsAssertionErrorOnDoubleFree started
[info] Test org.apache.spark.unsafe.PlatformUtilSuite.heapMemoryReuse started
[info] Test org.apache.spark.unsafe.PlatformUtilSuite.onHeapMemoryAllocatorPoolingReUsesLongArrays started
[info] Test org.apache.spark.unsafe.PlatformUtilSuite.onHeapMemoryAllocatorThrowsAssertionErrorOnDoubleFree started
[info] Test org.apache.spark.unsafe.PlatformUtilSuite.freeingOffHeapMemoryBlockResetsOffset started
[info] Test org.apache.spark.unsafe.PlatformUtilSuite.cleanerCreateMethodIsDefined started
[info] Test run org.apache.spark.unsafe.PlatformUtilSuite finished: 0 failed, 0 ignored, 9 total, 0.022s
[info] Passed: Total 9, Failed 0, Errors 0, Passed 9
[success] Total time: 7 s, completed Oct 13, 2023, 9:52:18 AM
```
--
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
Re: [PR] [SPARK-45508][CORE] Add "--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED" so Platform can access cleaner on Java 9+ [spark]
Posted by "JoshRosen (via GitHub)" <gi...@apache.org>.
JoshRosen commented on code in PR #43344:
URL: https://github.com/apache/spark/pull/43344#discussion_r1355953702
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/PlatformUtilSuite.java:
##########
@@ -157,4 +157,11 @@ public void heapMemoryReuse() {
Assertions.assertEquals(1024 * 1024 + 7, onheap4.size());
Assertions.assertEquals(obj3, onheap4.getBaseObject());
}
+
+ @Test
+ public void cleanerCreateMethodIsDefined() {
+ // Regression test for SPARK-45508: we don't expect the "no cleaner" fallback
+ // path to be hit in normal usage.
+ Assertions.assertTrue(Platform.cleanerCreateMethodIsDefined());
Review Comment:
If we want to backport this change then we will have to prepare a separate PR because https://github.com/apache/spark/pull/43074 changed the JUnit assertions helper names so the same source can't compile in all branches.
--
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
Re: [PR] [SPARK-45508][CORE] Add "--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED" so Platform can access Cleaner on Java 9+ [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #43344:
URL: https://github.com/apache/spark/pull/43344#issuecomment-1760887553
Merged into master for Spark 4.0. Thanks @JoshRosen and all ~
--
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
Re: [PR] [SPARK-45508][CORE] Add "--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED" so Platform can access Cleaner on Java 9+ [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #43344:
URL: https://github.com/apache/spark/pull/43344#issuecomment-1759866901
Could you re-trigger the two failed pipeline, @JoshRosen ?
--
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
Re: [PR] [SPARK-45508][CORE] Add "--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED" so Platform can access Cleaner on Java 9+ [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #43344:
URL: https://github.com/apache/spark/pull/43344#issuecomment-1760884710
```
[info] *** 1 TEST FAILED ***
[error] Failed tests:
[error] org.apache.spark.sql.kafka010.KafkaSourceStressSuite
[error] (sql-kafka-0-10 / Test / test) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 2147 s (35:47), completed Oct 13, 2023, 5:00:23 AM
```
This is a known flaky test, unrelated to this 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