-
Notifications
You must be signed in to change notification settings - Fork 28.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-44259][CONNECT][TESTS] Make connect-client-jvm
pass on Java 21 except RemoteSparkSession
-based tests
#41805
Conversation
RemoteSparkSession
for Java 21
Are there a lot of tests that fail with 21? I start to feel like we should just fix them instead of skipping individual tests. |
we can't fix them now, we need wait a new arrow version |
@HyukjinKwon All ignored tests related to apache/arrow#35053 and @dongjoon-hyun just fixed it, but arrow has not released a new version yet |
also cc @dongjoon-hyun FYI |
Yes, I fixed the root cause in Arrow-side. SPARK-44247 will upgrade to Apache Arrow 13.0.0 which will recover all ignored tests. The remaining problem is that Apache Arrow 13.0.0 release is expected on August. We will see. For this PR, I didn't choose this approach because this way was a little too intrusive. BTW, @LuciferYang .
|
RemoteSparkSession
for Java 21 connect-client-jvm
pass on Java 21 except RemoteSparkSession
-based tests
Already removed ~ |
@LuciferYang . |
Never mind~ |
+1 from my side. However, please follow @HyukjinKwon 's opinion. |
Please wait a moment, let me try a new way to reduce code changes |
* SPARK-44259: override test function to skip `RemoteSparkSession-based` tests as default, | ||
* we should delete this function after SPARK-44121 is completed. | ||
*/ | ||
override protected def test(testName: String, testTags: Tag*)(testFun: => Any) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dongjoon-hyun @HyukjinKwon the new code override test
function, all RemoteSparkSession-based
tests using Java 21 are ignored by default, so there is no need to add the assume condition to test case one by one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this seem less intrusive ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM. Yes, this looks much better.
Merged to master for Apache Spark 3.5.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM2
while java Java(TM) SE Runtime Environment (build 21.0.2+13-LTS-58)
what is the solution for this : Arrow can work with Aparch spark 3.5.0 in java 21 code :
|
@Midhunpottammal Spark 3.5 has not announced support for Java 21, this feature is likely to be released in Spark 4.0 :) |
Ya, @LuciferYang is right. To @Midhunpottammal , you need SPARK-43831 for Java 21 support. |
@dongjoon-hyun @LuciferYang Thank you, I experimented with different versions of Java, Spark, and Arrow.I managed to get Arrow working with a lower version of Java in Spark 3.5.0. Here's my stack:
When I try to move to Java version 21, I encounter the same error |
@Midhunpottammal As @dongjoon-hyun said, all the relevant patches in SPARK-43831 are needed for Java 21 support. So this is not a job that can be accomplished with minor changes on Spark 3.5. |
What changes were proposed in this pull request?
This pr ignore all tests inherit
RemoteSparkSession
as default for Java 21 by override thetest
function inRemoteSparkSession
, they are all arrow-based tests due to the use of arrow data format for rpc communication in connect.All ignored test related to apache/arrow#35053, so we should wait for upgrading to a new arrow version and re-enable them for Java 21, the following TODO JIRA is created for that.
Why are the changes needed?
Make Java 21 daily test can monitor other non-arrow based tests.
Does this PR introduce any user-facing change?
No
How was this patch tested?