Thanks Great. Do that mean you're a +1?
On Tue, 11 Jun 2019 at 21:46, Colin McCabe <cmcc...@apache.org> wrote: > On Tue, Jun 11, 2019, at 12:12, Andy Coates wrote: > > Thanks for the response Colin, > > > > > What specific benefits do we get from transitioning to using an > interface > > > rather than an abstract class? > > > > This is covered in the KLIP: "An AdminClient interface has several > > advantages over an abstract base class, most notably allowing > > multi-inheritance and the use of dynamic proxies" > > Hi Andy, > > I was not that familiar with dynamic proxies, which is why the advantages > weren't clear to me. I had a conversation offline with Ismael and he > explained some of the background here. > > The JDK includes a java.lang.reflect.Proxy class which can be used to > create dynamic proxies, but only for interfaces -- not for abstract > classes. This is just a limitation in the library, though-- there are > other ways of creating a dynamic proxy, like using cglib, that work with > abstract classes as well as with interfaces. But using something like > cglib involves taking an external dependency, which could be annoying. > > > > > > If we are serious about doing this, would it be cleaner to just change > > > AdminClient from an abstract class to an interface in Kafka 3.0? It > would > > > break binary compatibility, but you have to break binary compatibility > in > > > any case to get what you want here (no abstract class). And it would > have > > > the advantage of not creating a lot of churn in the codebase as people > > > replaced "AdminClient" with "Admin" all over the place. What do you > think? > > > > How far off is Kafka 3.0? This is causing us pain right now on a regular > > basis and, from Ryanne's email above we're not alone. I'm not against > > making this a change in Kafka 3.0, but only if its imminent. > > I don't believe 3.0 is imminent. > > > > > > On a related note, one problem I've seen is that people will subclass > > > AdminClient for testing. Then, every time Kafka adds a new API, we > add a > > > new abstract method to the base class, which breaks compilation for > them. > > > Their test classes would have been fine simply failing when the new > API was > > > called. So perhaps one useful class would be a class that implements > the > > > AdminClient API, and throws UnimplementedException for every method. > The > > > test classes could subclass this method and never have to worry about > new > > > methods getting added again. > > > > This is similar to what KSQL does for the other Kafka client APIs, except > > we use a dynamic proxy, rather than having to hand code the exception > > throwing. > > There is some performance penalty for the dynamic proxy approach, but it > certainly is simpler. > > With this context in mind, the change sounds reasonable to me. > > best, > Colin > > > > > > Another pattern I've seen is people wanting to implement a class which > is > > > similar to KafkaAdminClient in every way, except for the behavior of > > > close(). Specifically, people want to implement reference counting in > > > order to reuse AdminClient instances. So they start by implementing > > > essentially a delegating class, which just forwards every method to an > > > underlying AdminClient instance. But this has the same problem that it > > > breaks every time the upstream project adds an API. In order to > support > > > this, we could have an official DelegatingAdminClient base class that > > > forwarded every method to an underlying AdminClient instance. Then the > > > client code could override the methods they needed, like close. > > > > Again, this would be trivial to implement using dynamic proxies. No need > > for us to implement any `DelegatingAdminClient`. If this is an interface > we > > empower users to do this for themselves. > > > > best, > > Colin > > > > On Mon, 10 Jun 2019 at 21:44, Colin McCabe <cmcc...@apache.org> wrote: > > > > > Hi Andy, > > > > > > This is a big change, and I don't think there has been a lot of > discussion > > > about the pros and cons. What specific benefits do we get from > > > transitioning to using an interface rather than an abstract class? > > > > > > If we are serious about doing this, would it be cleaner to just change > > > AdminClient from an abstract class to an interface in Kafka 3.0? It > would > > > break binary compatibility, but you have to break binary compatibility > in > > > any case to get what you want here (no abstract class). And it would > have > > > the advantage of not creating a lot of churn in the codebase as people > > > replaced "AdminClient" with "Admin" all over the place. What do you > think? > > > > > > On a related note, one problem I've seen is that people will subclass > > > AdminClient for testing. Then, every time Kafka adds a new API, we > add a > > > new abstract method to the base class, which breaks compilation for > them. > > > Their test classes would have been fine simply failing when the new > API was > > > called. So perhaps one useful class would be a class that implements > the > > > AdminClient API, and throws UnimplementedException for every method. > The > > > test classes could subclass this method and never have to worry about > new > > > methods getting added again. > > > > > > Another pattern I've seen is people wanting to implement a class which > is > > > similar to KafkaAdminClient in every way, except for the behavior of > > > close(). Specifically, people want to implement reference counting in > > > order to reuse AdminClient instances. So they start by implementing > > > essentially a delegating class, which just forwards every method to an > > > underlying AdminClient instance. But this has the same problem that it > > > breaks every time the upstream project adds an API. In order to > support > > > this, we could have an official DelegatingAdminClient base class that > > > forwarded every method to an underlying AdminClient instance. Then the > > > client code could override the methods they needed, like close. > > > > > > best, > > > Colin > > > > > > > > > On Tue, Jun 4, 2019, at 09:17, Andy Coates wrote: > > > > Hi folks > > > > > > > > As there's been no chatter on this KIP I'm assuming it's > non-contentious, > > > > (or just boring), hence I'd like to call a vote for KIP-476: > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-476%3A+Add+Java+AdminClient+Interface > > > > > > > > Thanks, > > > > > > > > Andy > > > > > > > > > >
