Re: [PR] [SPARK-45508][CORE] Add "--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED" so Platform can access Cleaner on Java 9+ [spark]

2023-10-13 Thread via GitHub 


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-unsubscr...@spark.apache.org

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


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@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]

2023-10-13 Thread via GitHub 


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-unsubscr...@spark.apache.org

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


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@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]

2023-10-12 Thread via GitHub 


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-unsubscr...@spark.apache.org

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


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@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]

2023-10-12 Thread via GitHub 


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-unsubscr...@spark.apache.org

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


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@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]

2023-10-12 Thread via GitHub 


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-unsubscr...@spark.apache.org

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


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@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]

2023-10-12 Thread via GitHub 


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-unsubscr...@spark.apache.org

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


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@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]

2023-10-11 Thread via GitHub 


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-unsubscr...@spark.apache.org

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


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@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]

2023-10-11 Thread via GitHub 


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-unsubscr...@spark.apache.org

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


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

[PR] [SPARK-45508][CORE] Add "--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED" so Platform can access cleaner on Java 9+ [spark]

2023-10-11 Thread via GitHub 


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-unsubscr...@spark.apache.org

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


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