micronaut-projects / micronaut-kafka

Integration between Micronaut and Apache Kafka
Apache License 2.0
86 stars 107 forks source link

Remove annotation `@Internal` from class `JsonObjectSerde` #849

Closed guillermocalvo closed 1 year ago

yawkat commented 1 year ago

@dstepanov added you since you added the annotation

sonarcloud[bot] commented 1 year ago

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 commented 1 year ago

Why does it need to be "public"?

guillermocalvo commented 1 year ago

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

Because documentation says it can be subclassed:

dstepanov commented 1 year ago

In what way would you do it?

guillermocalvo commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

guillermocalvo commented 1 year ago

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

@sdelamo @yawkat @dstepanov @graemerocher

dstepanov commented 1 year ago

If it’s a valid case then yes