I double checked everything and unfortunately even when I created internal and external representation of ConsumerGroupMetadata, this did not help. The reasons why are as follows:
1. ConsumerCoordinator is evaluating the onAssignment method from ConsumerPartitionAssignor which is a public interface - it means that even when we try to use some alternative internal implementation of ConsumerGroupMetadata in ConsumerCoordinator, it will fail at that point. 2. AsyncKafkaConsumer implements the public org.apache.kafka.clients.consumer.Consumer interface, so for implementing the ConsumerGroupMetadata groupMetadata() method, it has to be able to instantiate a ConsumerGroupMetadata object. Unfortunately, AsyncKafkaConsumer is in org.apache.kafka.clients.consumer.internals, so as you mentioned, package-private scope will not fly. You can see what I did here: https://github.com/pszymczyk/kafka/tree/static-constructor-poc Having all those things in mind making constructors private and providing static factory method is not doable and it cannot be considered as an alternative approach. śr., 2 kwi 2025 o 02:55 Matthias J. Sax <mj...@apache.org> napisał(a): > Thanks for the pointer. > > It seems the example would require to make the constructor of > `ConsumerGroupMetadata` package private though? If it's `private` as in > the example, `retyrb kafkaConsumer.groupMetadata()` would not be able to > return an instance, as the object is created inside `KafkaConsumer`, not > `ConsumerGroupMetadata` class. > > And it seems even package private won't work, because > `ConsumerGroupMetadata` is in a different package than the actual > internal classes of `KafkaConsumer` which create an object (there are in > `o.a.k.client.consumer.internal.` package, `AyncKafkaConsumer` and others) > > So I don't think that the static method approach would actually work? > > > > -Matthias > > > On 3/31/25 6:22 AM, Paweł Szymczyk wrote: > > @mj...@apache.org you can see an example in `Rejected Alternatives` > section > > in KIP (ConsumerGroupMetadata newInstance(KafkaConsumer<?,?> > kafkaConsumer > > method). > > > > I agree with you that making the class an inner class of `KafkaConsumer` > is > > quite big change and making ConsumerGroupMetadata an interface is good > > enough. > > > > pt., 28 mar 2025 o 01:18 Matthias J. Sax <mj...@apache.org> napisał(a): > > > >>> and providing a static create method that takes a kafka consumer > >>> object as a parameter or we can make the ConsumerGroupMetada inner not > >>> static class in KafkaConsumer > >> > >> Can you elaborate on the "static create method" -- frankly, it sounds a > >> little bit clumsy to me? > >> > >> I guess we could make the class an inner class of `KafkaConsumer`. > >> However, this case the "splash radius" of the change is much larger: > >> > >> - we need to deprecate the existing class entirely > >> - we also need to deprecate the `Consumer#groupMetadata()` and add a > >> new "overload" which would return the new (nested) version of > >> `ConsumerGroupMetada` (not sure how name this new "overload"...) > >> - we need to deprecate `Producer#sendOffsetsToTransaction()` and add > a > >> new overload which accepts the new (nested) version of > >> `ConsumerGroupMetada` > >> > >> I think, this splash radius is too large, and not worth the trouble. > >> > >> > >> > >> Thus, while I agree that from a green field POV in interface is not > >> ideal (and using a private constructor and `final` class would be > >> better), we need to work with what we have, and overall to me it still > >> seems to be the best way forward is using an interface? -- Maybe the > >> "static create method" can be a viable alternative, but would need to > >> understand it better before I can form an opinion about it. > >> > >> > >> -Matthias > >> > >> > >> On 3/25/25 11:44 PM, Paweł Szymczyk wrote: > >>> Hi Kirk > >>> > >>> PR with making constructors deprecated since 4.1.0 is ready to merge -> > >>> https://github.com/apache/kafka/pull/18977. > >>> > >>> wt., 25 mar 2025 o 01:24 Kirk True <k...@kirktrue.pro> napisał(a): > >>> > >>>> Hi Paweł, > >>>> > >>>> We can't change the existing (public) constructors to private in 4.1 > as > >>>> that would break users. Yes, we can argue that users shouldn't be > >>>> instantiating the ConsumerGroupMetadata in the first place, but as of > >> 4.0 > >>>> those constructors are part of the published API. > >>>> > >>>> For 4.1, at most we could mark them as deprecated. > >>>> > >>>> Thanks, > >>>> Kirk > >>>> > >>>> On Mon, Mar 24, 2025, at 9:24 AM, Paweł Szymczyk wrote: > >>>>> mj...@apache.org any update on that? Can we release it in 4.1.0? > >>>>> > >>>>> pt., 7 mar 2025 o 09:08 Paweł Szymczyk <pawel.szymczy...@gmail.com> > >>>>> napisał(a): > >>>>> > >>>>>> mj...@apache.org the main point is that this class is a regular > data > >>>>>> structure, there is no way to give an alternative implementation > which > >>>> is > >>>>>> the purpose of interface in Java. We can completely eliminate the > >>>>>> possibility of creating an instance of this class in any other way > >>>> than via > >>>>>> Kafka Consumer by changing the access modifiers of the constructors > to > >>>>>> private and providing a static create method that takes a kafka > >>>> consumer > >>>>>> object as a parameter or we can make the ConsumerGroupMetada inner > not > >>>>>> static class in KafkaConsumer, for those two alternative scenarios > we > >>>> have > >>>>>> to provide different inner class for ConsumerCoordinator and > >>>>>> AsyncKafkaConsumer and then translate inner into public in Kafka > >>>> Consumer > >>>>>> class, like that: > >>>>>> > >>>>>> public ConsumerGroupMetadata groupMetadata() { > >>>>>> return new > >>>> ConsumerGroupMetadata(delegate.groupMetadata().groupId, > >>>>>> delegate.groupMetadata().generationId(), > >>>> delegate.groupMetadata().memberId, > >>>>>> groupMetadata().groupInstanceId); > >>>>>> } > >>>>>> > >>>>>> śr., 5 mar 2025 o 21:01 Matthias J. Sax <mj...@apache.org> > >> napisał(a): > >>>>>> > >>>>>>> Reversing the question. What is the problem with using an > interface? > >>>> It > >>>>>>> seem, and interface would provide the cleanest way to separate > public > >>>>>>> and internal things? Or maybe I misunderstand what you propose? > >>>>>>> > >>>>>>> We can for sure not have a `private` constructor. > >>>>>>> > >>>>>>> We can also not have "package private" constructor, because the > class > >>>> is > >>>>>>> public, but we instantiate it from multiple different (internal) > >>>> packages. > >>>>>>> > >>>>>>> `protected` could maybe work, but `protected` it technically still > >>>>>>> public API, so if we need to change the constructor, we would need > to > >>>> do > >>>>>>> a KIP. > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> -Matthias > >>>>>>> > >>>>>>> On 3/5/25 9:22 AM, Ismael Juma wrote: > >>>>>>>> Hi, > >>>>>>>> > >>>>>>>> Thanks for the KIP. Why did we reject the alternative where the > >>>>>>> constructor > >>>>>>>> is unavailable to external users? > >>>>>>>> > >>>>>>>> Ismael > >>>>>>>> > >>>>>>>> On Tue, Feb 25, 2025 at 8:27 AM Paweł Szymczyk < > >>>>>>> pawel.szymczy...@gmail.com> > >>>>>>>> wrote: > >>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>> > >>>> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1136%3A+Make+ConsumerGroupMetadata+an+interface > >>>>>>>>> > >>>>>>>>> -- > >>>>>>>>> Pozdrawiam > >>>>>>>>> Paweł Szymczyk > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>>> > >>>>>> > >>>>>> -- > >>>>>> Pozdrawiam > >>>>>> Paweł Szymczyk > >>>>>> > >>>>> > >>>>> > >>>>> -- > >>>>> Pozdrawiam > >>>>> Paweł Szymczyk > >>>>> > >>>> > >>> > >>> > >> > >> > > > > -- Pozdrawiam Paweł Szymczyk
