Hi Matthias, You're right and I agree with your comments.
Paweł—I'm happy with the KIP given Matthias’ points. Thanks, Kirk On Tue, Mar 4, 2025, at 12:20 PM, Matthias J. Sax wrote: > Hi, > > I think it should be sufficient to just state in the JavaDocs of the new > interface, that the interface is not intended to be implement by users, > and point to the Consumer method that returns it. > > To me, the main thing is really, that currently user can call `new` what > makes it easy to do thing wrong. If we change it to an interface, users > cannot call `new`, and if the interface says "do not implement me", it > should hopefully trigger the right response. > > In the end, we can only do so much to guide users, and there is some > remaining responsibility to use the software as intended that we cannot > lift of them. > > I don't think we would need a runtime check, but again, it's more of an > impl detail, not something the KIP has to cover? > > > About naming: I agree that `DefaultConsumerGroupMetadata` is not a > default, but it's only an internal class that users should not really > see, so the name does not matter too much, and we could also rename it > at any point. Of course, could also just use `ConsumerGroupMetadataImpl` > instead to avoid the term "default". > > Technically, the KIP does not even need to mention > `DefaultConsumerGroupMetadata` at all (or only very briefly). It's not > public API. > > > > > -Matthias > > On 3/1/25 12:35 PM, Paweł Szymczyk wrote: > > Hi Kirk > > Many thanks for very good questions! > > > > KT1. I don't think that internal implementation should totally prevent of > > passing alternative implementation, but for sure we need to log warning > > when producer receives different than default implementaion. I think that > > checking internally is the class implementation exactly > > DefaultConsumerGroupMetadata is against some basic principles like liskov > > substition and if we like to design this mechanism that way, we shouldn't > > expose an interface. > > KT2. I am also not 100% satisfied with that name, I don't think that there > > will be need for future different implementations as it is simple data > > struct. > > KT3. It will be place in producer/internals package and default java scope > > for constructor and whole class. > > > > @Matthias J. Sax <mj...@apache.org> having those KT1 and KT2 in mind, > > should we move forward with an interface or maybe ConsumerGroupMetadata has > > to be a class, but we have to limit possibility to create an instance > > manualy by making the class final and changing constructor visibility to > > private or package scope? > > > > czw., 27 lut 2025 o 01:51 Kirk True <k...@kirktrue.pro> napisał(a): > > > >> Hi Paweł, > >> > >> Thanks for the KIP! > >> > >> My questions: > >> > >> KT1. What will prevent developers from implementing their own > >> ConsumerGroupMetadata and passing that to sendOffsetsToTransaction()? I > >> assume the code will check the incoming object is of type > >> DefaultConsumerGroupMetadata? > >> > >> KT2. To me, the use of the adjective "default" in > >> DefaultConsumerGroupMetadata implies that there could be other > >> implementations. Is the intention that there could be other implementations > >> in the future? > >> > >> KT3. DefaultConsumerGroupMetadata should be defined in an "internals" > >> package of some sort, right? Will users ever reference the implementation > >> class name in their code? I'm assuming not. > >> > >> Thanks! > >> Kirk > >> > >> On Tue, Feb 25, 2025, at 8:24 AM, Paweł Szymczyk wrote: > >>> > >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1136%3A+Make+ConsumerGroupMetadata+an+interface > >>> > >>> -- > >>> Pozdrawiam > >>> Paweł Szymczyk > >>> > >> > > > > > >
