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

Remove annotation @Internal from class JsonObjectSerde #849

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

guillermocalvo
Copy link
Contributor

No description provided.

@guillermocalvo guillermocalvo added the type: bug Something isn't working label Aug 30, 2023
@guillermocalvo guillermocalvo self-assigned this Aug 30, 2023
@guillermocalvo guillermocalvo linked an issue Aug 30, 2023 that may be closed by this pull request
@yawkat yawkat requested a review from dstepanov August 30, 2023 08:59
@yawkat
Copy link
Member

yawkat commented Aug 30, 2023

@dstepanov added you since you added the annotation

@sonarcloud
Copy link

sonarcloud bot commented Aug 30, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@dstepanov
Copy link
Contributor

Why does it need to be "public"?

@guillermocalvo
Copy link
Contributor Author

@dstepanov Why does it need to be "public"?

Because documentation says it can be subclassed:

@dstepanov
Copy link
Contributor

In what way would you do it?

@guillermocalvo
Copy link
Contributor Author

In what way would you do it?

I'd say either we stop discouraging users from using this class (ie. remove @Internal), or we stop advertising it in the documentation (ie. remove that little ℹ️ box).

It all depends on what motivated the addition of this annotation in the first place. @dstepanov What was the rationale behind it?

@dstepanov
Copy link
Contributor

I just don’t understand how it can be extended and used to register anything. If you thinks the use case is valid go ahead and remove the internal. My motivation was to not expose internal classes as over API.

@guillermocalvo
Copy link
Contributor Author

My motivation was to not expose internal classes as over API.

If you believe this is really an internal class that shouldn't be directly subclassed by users, I can just remove that part of the guide. I'm good either way. Your call @dstepanov

Thoughts? @yawkat @sdelamo

@yawkat
Copy link
Member

yawkat commented Aug 30, 2023

it seems to me that to customize kafka serialization, subclassing JsonObjectSerde and overriding the read/write methods makes sense. but we have no sample of this.

@graemerocher you wrote this initially, do you remember how it's supposed to be used?

@henriquelsmti can you share what you do in your subclass?

@henriquelsmti
Copy link
Contributor

henriquelsmti commented Oct 4, 2023

@yawkat after some refactoring, we'r using JsonObjectSerde only in kafka streams, as a superclass for Serdes of specific types.

@guillermocalvo
Copy link
Contributor Author

There seem to be valid use cases for subclassing JsonObjectSerde. Shall we remove @Internal then?

@sdelamo @yawkat @dstepanov @graemerocher

@dstepanov
Copy link
Contributor

If it’s a valid case then yes

@sdelamo sdelamo merged commit 6c291b2 into master Nov 9, 2023
8 checks passed
@sdelamo sdelamo deleted the 637-jsonobjectserde-is-internal branch November 9, 2023 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

JsonObjectSerde is internal?
5 participants