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

[SPARK-49147][CORE] Mark KryoRegistrator with DeveloperApi interface #47657

Closed
wants to merge 4 commits into from

Conversation

robreeves
Copy link
Contributor

@robreeves robreeves commented Aug 7, 2024

What changes were proposed in this pull request?

Trait org.apache.spark.serializer.KryoRegistrator is a public interface because it is exposed via config spark.kryo.registrator, but it is not annotated to describe the stability level. This adds the DeveloperApi annotation to formalize the contract.

Why are the changes needed?

This will help users understand the compatibility guarantees when using the trait. It is also helpful for scripts that need to identify classes with public APIs. We recently missed this trait in a script used to analyze which namespaces can be shaded.

Does this PR introduce any user-facing change?

Yes, it adds the DeveloperApi interface to the trait.

How was this patch tested?

N/A. Annotation change only.

Was this patch authored or co-authored using generative AI tooling?

No

@github-actions github-actions bot added the CORE label Aug 7, 2024
@robreeves
Copy link
Contributor Author

@dongjoon-hyun @mridulm please take a look

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-49147][CORE] Mark KryoSerializer as a stable interface [SPARK-49147][CORE] Mark KryoSerializer as a stable interface Aug 7, 2024
@robreeves robreeves changed the title [SPARK-49147][CORE] Mark KryoSerializer as a stable interface [SPARK-49147][CORE] Mark KryoRegistrator as a stable interface Aug 7, 2024
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Aug 7, 2024

In addition, this cannot be Stable because JavaSerializer is DeveloperApi.

@DeveloperApi
class JavaSerializer(conf: SparkConf) extends Serializer with Externalizable {

If we want to annotate, this should have DeveloperApi in the similar way.

@robreeves robreeves changed the title [SPARK-49147][CORE] Mark KryoRegistrator as a stable interface [SPARK-49147][CORE] Mark KryoRegistrator with DeveloperApi interface Aug 7, 2024
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Since it's a documented for a long time, I agree with the proposal.
https://spark.apache.org/docs/latest/api/scala/org/apache/spark/serializer/KryoRegistrator.html

Screenshot 2024-08-07 at 16 04 36

+1, LGTM. Thank you, @robreeves .

@mridulm
Copy link
Contributor

mridulm commented Aug 8, 2024

Do we want to make both of these @Stable @dongjoon-hyun ? These interfaces have been around since 0.x days :-)

In general, we have rarely promoted any of our DeveloperApi api to Stable - something to look into for 4.0 ?

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Aug 8, 2024

If you want, we can do, of course, @mridulm . :)

Do we want to make both of these @Stable

@yaooqinn
Copy link
Member

yaooqinn commented Aug 8, 2024

May it be broken if we migrate Kryo from 4.x to 5.x?

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Aug 8, 2024

May it be broken if we migrate Kryo from 4.x to 5.x?

I didn't see any breaking change in public API of Kryo.java. Do you mean that you know something intrusive recently, @yaooqinn ?

And, we depend on twitter-chill which has no change since 2021. I thought we don't have any plan to move out from this.

Screenshot 2024-08-07 at 19 55 51

@yaooqinn
Copy link
Member

yaooqinn commented Aug 8, 2024

Hi @dongjoon-hyun the question is just for curiosity to make it Stable or DeveloperAPI

Thank you for the explanation.

@pan3793
Copy link
Member

pan3793 commented Aug 8, 2024

twitter-chill made some progress in migrating to Kryo5 twitter/chill#747

@mridulm
Copy link
Contributor

mridulm commented Aug 8, 2024

Thinking more, let us decouple DeveloperApi -> Stable from this PR.
IMO we should revisit our public api and reclassify stable developer API's as stable - but that is a more cross cutting effort (and this specific one can be revisited in that context)

@dongjoon-hyun
Copy link
Member

Sounds good to me. +1 for merging this PR in the AS-IS status.

BTW, could you re-trigger the CI, @robreeves ?

@yaooqinn
Copy link
Member

yaooqinn commented Aug 8, 2024

The docker test failure is irrelevant. Merged to master.

Thank you @robreeves @dongjoon-hyun @mridulm @pan3793

@yaooqinn yaooqinn closed this in b54b300 Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants