Skip to content
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

[ISSUE-342][Improvement] Check Spark Serializer type #344

Merged
merged 4 commits into from
Nov 21, 2022

Conversation

chong0929
Copy link
Contributor

What changes were proposed in this pull request?

Spark have multiple serializers. We support the spark serializer which supportsRelocationOfSerializedObjects.
You can see https://github.com/apache/spark/blob/25849684b78cca6651e25d6efc9644a576e7e20f/core/src/main/scala/org/apache/spark/serializer/Serializer.scala#L98

Spark have three kinds of serializer
org.apache.spark.serializer.JavaSerializer
org.apache.spark.sql.execution.UnsafeRowSerializer
org.apache.spark.serializer.KryoSerializer
Only org.apache.spark.serializer.JavaSerializer don't support RelocationOfSerializedObjects.

Why are the changes needed?

So when we find the parameters to use org.apache.spark.serializer.JavaSerializer, We should throw an exception.

Does this PR introduce any user-facing change?

No

How was this patch tested?

test locally

@jerqi jerqi linked an issue Nov 20, 2022 that may be closed by this pull request
3 tasks
@jerqi jerqi changed the title [Improvement] Check Spark Serializer type [ISSUE-342][Improvement] Check Spark Serializer type Nov 20, 2022
@chong0929
Copy link
Contributor Author

@jerqi Hi, would you mind to take a look?

@jerqi
Copy link
Contributor

jerqi commented Nov 20, 2022

@jerqi Hi, would you mind to take a look?

Please fix code style and compile error.

@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2022

Codecov Report

Merging #344 (4c2422e) into master (d09b40b) will increase coverage by 0.04%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #344      +/-   ##
============================================
+ Coverage     58.17%   58.22%   +0.04%     
  Complexity     1529     1529              
============================================
  Files           192      191       -1     
  Lines         10606    10597       -9     
  Branches        924      924              
============================================
  Hits           6170     6170              
+ Misses         4068     4059       -9     
  Partials        368      368              
Impacted Files Coverage Δ
...ava/org/apache/uniffle/common/RssShuffleUtils.java

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jerqi
Copy link
Contributor

jerqi commented Nov 20, 2022

you can click the failure pipeline https://github.com/apache/incubator-uniffle/actions/runs/3507622551/jobs/5875494928 and then you can see
企业微信截图_c3d23905-9da7-4e2f-ba69-43105d61cfcb
You should fix these code style issues.

@jerqi
Copy link
Contributor

jerqi commented Nov 20, 2022

We should modify the test org.apache.uniffle.test.GetReaderTest, the test use the JavaSerializer. We should change it. https://github.com/apache/incubator-uniffle/actions/runs/3507622551/jobs/5875494954

@zuston
Copy link
Member

zuston commented Nov 20, 2022

you can click the failure pipeline https://github.com/apache/incubator-uniffle/actions/runs/3507622551/jobs/5875494928 and then you can see 企业微信截图_c3d23905-9da7-4e2f-ba69-43105d61cfcb You should fix these code style issues.

You could import the uniffle codestyle into the IDEA, following this guide https://github.com/apache/incubator-uniffle/blob/master/CONTRIBUTING.md#code-style-guide @chong0929

@chong0929
Copy link
Contributor Author

We should modify the test org.apache.uniffle.test.GetReaderTest, the test use the JavaSerializer. We should change it. https://github.com/apache/incubator-uniffle/actions/runs/3507622551/jobs/5875494954

Thanks for you review, add a default conf about spark.serializer which shoule be KryoSerializer when in PBS.

@chong0929
Copy link
Contributor Author

you can click the failure pipeline https://github.com/apache/incubator-uniffle/actions/runs/3507622551/jobs/5875494928 and then you can see 企业微信截图_c3d23905-9da7-4e2f-ba69-43105d61cfcb You should fix these code style issues.

You could import the uniffle codestyle into the IDEA, following this guide https://github.com/apache/incubator-uniffle/blob/master/CONTRIBUTING.md#code-style-guide @chong0929

Thanks you for your reminder and suggestion, I'm trying to make some changes.

@jerqi
Copy link
Contributor

jerqi commented Nov 20, 2022

Spark3 also have org.apache.uniffle.test.GetReaderTest, you should also modify it.

Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @chong0929 @zuston . Great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Check Spark Serializer type
4 participants