Hi all,
Just a quick note to let you all know that the KIP ran into a slight hiccup
along the way. The original change saw the return value of
`KafkaClientSupplier.getAdminClient` changed from `AdminClient` to the new
`Admin`, thereby allowing implementers to return a proxy is they so
wanted.
Awesome sauce - so I'd like to close the voting. final vote was:
4 for binding, none against
3 non-binding, none against.
I'll update the KIP to reflect the passing of the vote.
Thanks you all for your time & brain power,
Andy
On Thu, 11 Jul 2019 at 14:51, Rajini Sivaram
wrote:
> +1
+1 (binding)
Thanks for the KIP, Andy!
Regards,
Rajini
On Thu, Jul 11, 2019 at 1:18 PM Gwen Shapira wrote:
> +1 (binding)
>
> Thank you for the improvement.
>
> On Thu, Jul 11, 2019, 3:53 AM Andy Coates wrote:
>
> > Hi All,
> >
> > So voting currently stands on:
> >
> > Binding:
> > +1
+1 (binding)
Thank you for the improvement.
On Thu, Jul 11, 2019, 3:53 AM Andy Coates wrote:
> Hi All,
>
> So voting currently stands on:
>
> Binding:
> +1 Matthias,
> +1 Colin
>
> Non-binding:
> +1 Thomas Becker
> +1 Satish Guggana
> +1 Ryan Dolan
>
> So we're still 1 binding vote short. :(
Hi All,
So voting currently stands on:
Binding:
+1 Matthias,
+1 Colin
Non-binding:
+1 Thomas Becker
+1 Satish Guggana
+1 Ryan Dolan
So we're still 1 binding vote short. :(
On Wed, 3 Jul 2019 at 23:08, Matthias J. Sax wrote:
> Thanks for the details Colin and Andy.
>
> My indent was not to
Thanks for the details Colin and Andy.
My indent was not to block the KIP, but it seems to be a fair question
to ask.
I talked to Ismael offline about it and understand his reasoning better
now. If we don't deprecate `abstract AdminClient` class, it seems
reasonable to not deprecate the
Matthias,
I was referring to platforms such as spark or flink that support multiple
versions of the Kafka clients. Ismael mentioned this higher up on the
thread.
I'd prefer this KIP didn't get held up over somewhat unrelated change, i.e.
should the factory method be on the interface or utility
On Tue, Jul 2, 2019, at 09:14, Colin McCabe wrote:
> On Mon, Jul 1, 2019, at 23:30, Matthias J. Sax wrote:
> > Not sure, if I understand the argument?
> >
> > Why would anyone need to support multiple client side versions?
> > Clients/brokers are forward/backward compatible anyway.
>
> When
On Mon, Jul 1, 2019, at 23:30, Matthias J. Sax wrote:
> Not sure, if I understand the argument?
>
> Why would anyone need to support multiple client side versions?
> Clients/brokers are forward/backward compatible anyway.
When you're using many different libraries, it is helpful if they don't
Not sure, if I understand the argument?
Why would anyone need to support multiple client side versions?
Clients/brokers are forward/backward compatible anyway.
Also, if one really supports multiple client side versions, won't they
use multiple shaded dependencies for different versions?
Last,
Done. I've not deprecated the factory methods on the AdminClient for the
same reason the AdminClient itself is not deprecated, i.e. this would cause
unavoidable warnings for libraries / platforms that support multiple
versions of Kafka. However, I think we add a note to the Java docs of
Done.
On Thu, 27 Jun 2019 at 23:21, Matthias J. Sax wrote:
> @Andy:
>
> What about the factory methods of `AdminClient` class? Should they be
> deprecated?
>
> One nit about the KIP: can you maybe insert "code blocks" to highlight
> the API changes? Code blocks would simplify to read the KIP a
@Andy:
What about the factory methods of `AdminClient` class? Should they be
deprecated?
One nit about the KIP: can you maybe insert "code blocks" to highlight
the API changes? Code blocks would simplify to read the KIP a lot.
-Matthias
On 6/26/19 6:56 AM, Ryanne Dolan wrote:
> +1
+1 (non-binding)
Thanks.
Ryanne
On Tue, Jun 25, 2019 at 10:21 PM Satish Duggana
wrote:
> +1 (non-binding)
>
> On Wed, Jun 26, 2019 at 8:37 AM Satish Duggana
> wrote:
> >
> > +1 Matthias/Andy.
> > IMHO, interface is about the contract, it should not have/expose any
> > implementation. I am
+1 (non-binding)
On Wed, Jun 26, 2019 at 8:37 AM Satish Duggana wrote:
>
> +1 Matthias/Andy.
> IMHO, interface is about the contract, it should not have/expose any
> implementation. I am fine with either way as it is more of taste or
> preference.
>
> Agree with Ismael/Colin/Ryanne on not
+1 Matthias/Andy.
IMHO, interface is about the contract, it should not have/expose any
implementation. I am fine with either way as it is more of taste or
preference.
Agree with Ismael/Colin/Ryanne on not deprecating for good reasons.
On Mon, Jun 24, 2019 at 8:33 PM Andy Coates wrote:
>
> I
+1 (binding).
C.
On Mon, Jun 24, 2019, at 08:10, Andy Coates wrote:
> Hi all,
>
> KIP updated:
> - No deprecation
> - Factory method back onto Admin interface
>
> I'd like to kick off another round of voting please.
>
> Thanks,
>
> Andy
>
> On Mon, 24 Jun 2019 at 16:03, Andy Coates wrote:
Hi all,
KIP updated:
- No deprecation
- Factory method back onto Admin interface
I'd like to kick off another round of voting please.
Thanks,
Andy
On Mon, 24 Jun 2019 at 16:03, Andy Coates wrote:
> I agree Matthias.
>
> (In Scala, such factory methods are on a companion object. As Java
I agree Matthias.
(In Scala, such factory methods are on a companion object. As Java doesn't
have the concept of a companion object, an equivalent would be a utility
class with a similar name...)
However, I'll update the KIP to include the factory method on the interface.
On Fri, 21 Jun 2019 at
That makes a lot of sense. OK, no deprecation.
On Fri, 21 Jun 2019 at 15:11, Ismael Juma wrote:
> This is even more reason not to deprecate immediately, there is very little
> maintenance cost for us. We should be mindful that many of our users (eg
> Spark, Flink, etc.) typically allow users
Just to give a little context here, the main reason for having the
AdminClient#create method is so that end-users didn't have to import
KafkaAdminClient. In general, users should be interacting with the AdminClient
API, not with the implementation class(es).
Also, I have to grudgingly agree
I still think, that an interface does not need to know anything about
its implementation. But I am also fine if we add a factory method to the
new interface if that is preferred by most people.
-Matthias
On 6/21/19 7:10 AM, Ismael Juma wrote:
> This is even more reason not to deprecate
This is even more reason not to deprecate immediately, there is very little
maintenance cost for us. We should be mindful that many of our users (eg
Spark, Flink, etc.) typically allow users to specify the kafka clients
version and hence avoid using new classes/interfaces for some time. They
would
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
Hi Ismael,
I’m happy enough to not deprecate the existing `AdminClient` class as part of
this change.
However, note that, the class will likely be empty, i.e. all methods and
implementations will be inherited from the interface:
public abstract class AdminClient implements Admin {
}
Not
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
I agree with Ryanne, I think we should avoid deprecating AdminClient and
causing so much churn for users who don't actually care about this niche
use case.
Ismael
On Tue, Jun 18, 2019 at 6:43 AM Andy Coates wrote:
> Hi Ryanne,
>
> If we don't change the client code, then everywhere will still
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 wrote:
> Hi All,
>
> I've updated
Hi Ryanne,
If we don't change the client code, then everywhere will still expect
subclasses of `AdminClient`, so the interface will be of no use, i.e. I
can't write a class that implements the new interface and pass it to the
client code.
Thanks,
Andy
On Mon, 17 Jun 2019 at 19:01, Ryanne Dolan
Andy, while I agree that the new interface is useful, I'm not convinced
adding an interface requires deprecating AdminClient and changing so much
client code. Why not just add the Admin interface, have AdminClient
implement it, and have done?
Ryanne
On Mon, Jun 17, 2019 at 12:09 PM Andy Coates
Hi all,
I think I've addressed all concerns. Let me know if I've not. Can I call
another round of votes please?
Thanks,
Andy
On Fri, 14 Jun 2019 at 04:55, Satish Duggana
wrote:
> Hi Andy,
> Thanks for the KIP. This is a good change and it gives the user a better
> handle on Admin client
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...
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
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
With 3.0 not imminent I would prefer to make this change soon, rather than
later.
On Tue, 11 Jun 2019 at 21:46, Colin McCabe 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
>
Thanks Great.
Do that mean you're a +1?
On Tue, 11 Jun 2019 at 21:46, Colin McCabe 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
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
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
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
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.
+1 non-binding
We've run into issues trying to decorate the AdminClient due it being an
abstract class. Will be nice to have consistency with Producer/Consumer as well.
On Tue, 2019-06-04 at 17:17 +0100, Andy Coates wrote:
Hi folks
As there's been no chatter on this KIP I'm assuming it's
> 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
43 matches
Mail list logo
