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 > > > > > >
