Hi Andy, I didn't see any reason being mentioned why it's an anti pattern or cleaner so I assume it's subjective. :) For what it's worth, the approach of a collection interface providing a default implementation is very common in Scala and it makes a lot of sense in my mind. For example, why does everyone have to decide the List implementation, ArrayList is the best default and it should just be chosen for the common case.
Having a separate class with the same name and a suffix at the end just adds complexity and surface area while making it less discoverable for very little benefit in my opinion. Ismael On Fri, Jun 21, 2019 at 3:56 AM Andy Coates <a...@confluent.io> wrote: > Hi Ismael, > > Matthias thought having the interface also provide a factory method that > returns a specific implementation was a bit of an anti-pattern, and I would > tend to agree, though I’ve used this same pattern myself at times where the > set of implementations is known. > > Matthias may want to answer with his own thoughts, but from my own > experience I’ve often found it cleaner to have a companion utility class to > an interface in an API, that has factory methods to create instances of the > common set of implementations. In this case it would be `KafkaAdminClient` > only at the moment, though if we were to support something like the > `DelegatingAdminClient` that Colin mentioned in the future, this too could > be on the utility class. > > My preference is the separate classes. But in the interest of moving this > forward I’m happy with either. > > Andy > > > > > On 18 Jun 2019, at 16:07, Ismael Juma <ism...@juma.me.uk> wrote: > > > > I don't agree with this change. The idea that an interface cannot have a > > default implementation is outdated in my view. Can someone provide any > > benefit to introducing a separate class for the factory method? > > > > Ismael > > > > On Mon, Jun 17, 2019 at 10:07 AM Andy Coates <a...@confluent.io> wrote: > > > >> Hi All, > >> > >> I've updated the KIP to move the `create` factory method implementation > >> into a new `AdminClients` utility class, rather than on the new `Admin` > >> interface. > >> > >> Satish, > >> > >> As above, the KIP has been updated to only have the operations on the > >> `Admin` api. As for the overhead of dynamic proxies... the use of > dynamic > >> proxies is totally up to the users of the library. In KSQL we use > dynamic > >> proxies because the overhead is acceptable and it decouples us from > >> additions to the client interfaces. Others may decide otherwise for > their > >> project. By making the admin api an interface we're empowering users to > >> choose the right approach for them. > >> > >> This is the primary motivation for the KIP from my point of view. > However, > >> it also brings it inline with the other Kafka clients, and gives users > the > >> freedom to do what they want, rather than requiring the use of an > abstract > >> base class. > >> > >> Thanks, > >> > >> Andy > >> > >> > >> On Fri, 14 Jun 2019 at 04:55, Satish Duggana <satish.dugg...@gmail.com> > >> wrote: > >> > >>> Hi Andy, > >>> Thanks for the KIP. This is a good change and it gives the user a > better > >>> handle on Admin client usage. I agree with the proposal except the new > >>> `Admin` interface having all the methods from `AdminClient` abstract > >> class. > >>> It should be kept clean having only the admin operations as methods > from > >>> KafkaClient abstract class but not the factory methods as mentioned in > >> the > >>> earlier mail. > >>> > >>> I know about dynamic proxies(which were widely used in RMI/EJB world). > I > >> am > >>> curious about the usecase using dynamic proxies with Admin client > >>> interface. Dynamic proxy can have performance penalty if it is used in > >>> critical path. Is that the primary motivation for creating the KIP? > >>> > >>> Thanks, > >>> Satish. > >>> > >>> On Wed, Jun 12, 2019 at 8:43 PM Andy Coates <a...@confluent.io> wrote: > >>> > >>>> I'm not married to that part. That was only done to keep it more or > >> less > >>>> inline with what's already there, (an abstract class that has a > factory > >>>> method that returns a subclass.... sounds like the same anti-pattern > >> ;)) > >>>> > >>>> An alternative would to have an `AdminClients` utility class to create > >>> the > >>>> admin client. > >>>> > >>>> On Mon, 10 Jun 2019 at 19:31, Matthias J. Sax <matth...@confluent.io> > >>>> wrote: > >>>> > >>>>> Hmmm... > >>>>> > >>>>> So the new interface, returns an instance of a class that implements > >>> the > >>>>> interface. This sounds a little bit like an anti-pattern? Shouldn't > >>>>> interfaces actually not know anything about classes that implement > >> the > >>>>> interface? > >>>>> > >>>>> > >>>>> -Matthias > >>>>> > >>>>> On 6/10/19 11:22 AM, Andy Coates wrote: > >>>>>> `AdminClient` would be deprecated purely because it would no longer > >>>> serve > >>>>>> any purpose and would be virtually empty, getting all of its > >>>>> implementation > >>>>>> from the new interfar. It would be nice to remove this from the API > >>> at > >>>>> the > >>>>>> next major version bump, hence the need to deprecate. > >>>>>> > >>>>>> `AdminClient.create()` would return what it does today, (so not a > >>>>> breaking > >>>>>> change). > >>>>>> > >>>>>> On Tue, 4 Jun 2019 at 22:24, Ryanne Dolan <ryannedo...@gmail.com> > >>>> wrote: > >>>>>> > >>>>>>>> The existing `AdminClient` will be marked as deprecated. > >>>>>>> > >>>>>>> What's the reasoning behind this? I'm fine with the other changes, > >>> but > >>>>>>> would prefer to keep the existing public API intact if it's not > >>>> hurting > >>>>>>> anything. > >>>>>>> > >>>>>>> Also, what will AdminClient.create() return? Would it be a > >> breaking > >>>>> change? > >>>>>>> > >>>>>>> Ryanne > >>>>>>> > >>>>>>> On Tue, Jun 4, 2019, 11:17 AM Andy Coates <a...@confluent.io> > >>> 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 > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>>> > >>>> > >>> > >> > >
