You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Tom Bentley <t....@gmail.com> on 2017/09/04 11:54:48 UTC
Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand
to use AdminClient
The KIP has been adopted after a successful vote.
Unfortunately I've discovered that there's an annoying detail in the
handling of the case that electPreferredLeaders() is called with a null
partitions argument. As discussed with Ewen, this is supposed to mean "all
partitions", but we don't know all the partitions in the AdminClient, yet
we have to return a ElectPreferredLeadersResults instance, supposedly with
the partitions as keys.
We could handle this by passing a KafkaFuture<Map<TopicPartition,
KafkaFuture<Void>>> to the ctor of ElectPreferredLeadersResults, instead of
an extant Map<TopicPartition, KafkaFuture<Void>> (the API of
ElectPreferredLeadersResults would not change). In the case that the
partitions argument was not null this future will already be completed. In
the case where partitions argument was null this future will be completed
when we have a response from which we discover the partitions; in the
meantime the AdminClient can carry on being used for other calls. So in the
normal case there's not really a problem.
The problem comes where there's an exception *before we get the response*,
that means we still don't know the partitions to populate the map with. In
practice this would mean that an exception could propagate out of
ElectPreferredLeadersResults.values() rather than when the map was accessed
element-wise. Since we control the API of ElectPreferredLeadersResults we
could document that values() (and consequently all()) could throw,. We
could even use checked exceptions, though since the exception would only
happen in the case that the partitions argument was null that would feel
rather heavy-handed to me.
Another alternative would be to block in AdminClient.electPreferredLeaders()
in the case that the partitions argument was null, and if there was an
error let the exception propagate out of electPreferredLeaders() directly.
Sorry about having to ask about this once people have already voted, but
what do people think about these options?
Thanks,
Tom
On 30 August 2017 at 16:55, Tom Bentley <t....@gmail.com> wrote:
> I've updated in the KIP.
>
> Thanks,
>
> Tom
>
> On 30 August 2017 at 16:42, Ismael Juma <is...@juma.me.uk> wrote:
>
>> If you agree with the change, yes, please rename. It's OK to make changes
>> after the VOTE thread starts. In cases where some people have already
>> voted, it's recommended to mention the changes in the VOTE thread as a
>> heads up. Generally, we don't restart the vote unless the changes are
>> significant.
>>
>> Ismael
>>
>> On Wed, Aug 30, 2017 at 4:26 PM, Tom Bentley <t....@gmail.com>
>> wrote:
>>
>> > Hi Ismael,
>> >
>> > I agree that `electPreferredReplicaLeader` is a mouthful and am happy to
>> > change it to `electPreferredLeaders`. I'd rename the correspond request
>> and
>> > response similarly.
>> >
>> > Should I rename it in the KIP now, even though I initiated a VOTE thread
>> > yesterday?
>> >
>> > Cheers,
>> >
>> > Tom
>> >
>> > On 30 August 2017 at 16:01, Ismael Juma <is...@juma.me.uk> wrote:
>> >
>> > > Hi Tom,
>> > >
>> > > Thanks for the KIP, it's a useful one. I find the proposed method name
>> > > `electPreferredReplicaLeader` a little hard to read. It seems that a
>> > small
>> > > change would make it clearer: `electPreferredReplicaAsLeader`. The
>> next
>> > > point is that this is a batch API, so it should ideally be plural like
>> > the
>> > > other AdminClient methods. Maybe `electPreferredReplicasAsLeaders`,
>> but
>> > > that's quite a mouthful. Maybe we should shorten it to
>> > > `electPreferredLeaders`. Thoughts?
>> > >
>> > > Ismael
>> > >
>> > > On Wed, Aug 2, 2017 at 6:34 PM, Tom Bentley <t....@gmail.com>
>> > wrote:
>> > >
>> > > > In a similar vein to KIP-179 I've created KIP-183 (
>> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-183+-+Change+
>> > > > PreferredReplicaLeaderElectionCommand+to+use+AdminClient)
>> > > > which is about deprecating the --zookeeper option to
>> > > > kafka-preferred-replica-election.sh and replacing it with an option
>> > > which
>> > > > would use a new AdminClient-based API.
>> > > >
>> > > > As it stands the KIP is focussed on simply moving the existing
>> > > > functionality behind the AdminClient.
>> > > >
>> > > > I'd be grateful for any feedback people may have on this.
>> > > >
>> > > > Thanks,
>> > > >
>> > > > Tom
>> > > >
>> > >
>> >
>>
>
>
Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand
to use AdminClient
Posted by Tom Bentley <t....@gmail.com>.
Hi Jun,
The patches that I've got currently wait for the elections to complete
before returning the response. Is that the semantic you wanted?
Cheers,
Tom
On 7 September 2017 at 22:14, Jun Rao <ju...@confluent.io> wrote:
> Hi, Tom,
>
> It seems that it's useful to know whether the leader is balanced for each
> partition in ElectPreferredLeadersResult, instead of just being attempted?
>
> Thanks,
>
> Jun
>
> On Wed, Sep 6, 2017 at 4:03 PM, Colin McCabe <cm...@apache.org> wrote:
>
> > On Wed, Sep 6, 2017, at 01:18, Tom Bentley wrote:
> > > Hi Colin,
> > >
> > > Thanks for taking the time to respond.
> > >
> > > On 5 September 2017 at 22:22, Colin McCabe <cm...@apache.org> wrote:
> > >
> > > > ...
> > > > Why does there need to be a map at all in the API?
> > >
> > >
> > > From a purely technical PoV there doesn't, but doing something else
> would
> > > make the API inconsistent with other similar AdminClient *Results
> > > classes,
> > > which all expose a Map directly.
> > >
> > >
> > > > Why not just have
> > > > something like this:
> > > >
> > >
> > > I agree this would be a better solution. I will update the KIP and ask
> > > people to vote again. (Is that the right process?)
> > >
> > > It might be worth bearing this in mind for future AdminClient APIs:
> > > Exposing a Map directly means you can't retrofit handling a null
> argument
> > > to mean "all the things", whereas wrapping the map would allow that.
> >
> > That's a good point.
> >
> > I guess the important thing to keep in mind is that if you return a map
> > from a results class, it has to be instantiated eagerly. It has to be
> > something you know before any RPCs are made, async actions are
> > performed, etc.
> >
> > best,
> > Colin
> >
> > >
> > > Thanks again,
> > >
> > > Tom
> >
>
Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand
to use AdminClient
Posted by Jun Rao <ju...@confluent.io>.
Hi, Tom,
It seems that it's useful to know whether the leader is balanced for each
partition in ElectPreferredLeadersResult, instead of just being attempted?
Thanks,
Jun
On Wed, Sep 6, 2017 at 4:03 PM, Colin McCabe <cm...@apache.org> wrote:
> On Wed, Sep 6, 2017, at 01:18, Tom Bentley wrote:
> > Hi Colin,
> >
> > Thanks for taking the time to respond.
> >
> > On 5 September 2017 at 22:22, Colin McCabe <cm...@apache.org> wrote:
> >
> > > ...
> > > Why does there need to be a map at all in the API?
> >
> >
> > From a purely technical PoV there doesn't, but doing something else would
> > make the API inconsistent with other similar AdminClient *Results
> > classes,
> > which all expose a Map directly.
> >
> >
> > > Why not just have
> > > something like this:
> > >
> >
> > I agree this would be a better solution. I will update the KIP and ask
> > people to vote again. (Is that the right process?)
> >
> > It might be worth bearing this in mind for future AdminClient APIs:
> > Exposing a Map directly means you can't retrofit handling a null argument
> > to mean "all the things", whereas wrapping the map would allow that.
>
> That's a good point.
>
> I guess the important thing to keep in mind is that if you return a map
> from a results class, it has to be instantiated eagerly. It has to be
> something you know before any RPCs are made, async actions are
> performed, etc.
>
> best,
> Colin
>
> >
> > Thanks again,
> >
> > Tom
>
Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to
use AdminClient
Posted by Colin McCabe <cm...@apache.org>.
On Wed, Sep 6, 2017, at 01:18, Tom Bentley wrote:
> Hi Colin,
>
> Thanks for taking the time to respond.
>
> On 5 September 2017 at 22:22, Colin McCabe <cm...@apache.org> wrote:
>
> > ...
> > Why does there need to be a map at all in the API?
>
>
> From a purely technical PoV there doesn't, but doing something else would
> make the API inconsistent with other similar AdminClient *Results
> classes,
> which all expose a Map directly.
>
>
> > Why not just have
> > something like this:
> >
>
> I agree this would be a better solution. I will update the KIP and ask
> people to vote again. (Is that the right process?)
>
> It might be worth bearing this in mind for future AdminClient APIs:
> Exposing a Map directly means you can't retrofit handling a null argument
> to mean "all the things", whereas wrapping the map would allow that.
That's a good point.
I guess the important thing to keep in mind is that if you return a map
from a results class, it has to be instantiated eagerly. It has to be
something you know before any RPCs are made, async actions are
performed, etc.
best,
Colin
>
> Thanks again,
>
> Tom
Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand
to use AdminClient
Posted by Tom Bentley <t....@gmail.com>.
Hi Colin,
Thanks for taking the time to respond.
On 5 September 2017 at 22:22, Colin McCabe <cm...@apache.org> wrote:
> ...
> Why does there need to be a map at all in the API?
From a purely technical PoV there doesn't, but doing something else would
make the API inconsistent with other similar AdminClient *Results classes,
which all expose a Map directly.
> Why not just have
> something like this:
>
I agree this would be a better solution. I will update the KIP and ask
people to vote again. (Is that the right process?)
It might be worth bearing this in mind for future AdminClient APIs:
Exposing a Map directly means you can't retrofit handling a null argument
to mean "all the things", whereas wrapping the map would allow that.
Thanks again,
Tom
Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to
use AdminClient
Posted by Colin McCabe <cm...@apache.org>.
On Mon, Sep 4, 2017, at 04:54, Tom Bentley wrote:
> The KIP has been adopted after a successful vote.
Thanks for working on this, Tom. It's a nice improvement.
>
> Unfortunately I've discovered that there's an annoying detail in the
> handling of the case that electPreferredLeaders() is called with a null
> partitions argument. As discussed with Ewen, this is supposed to mean
> "all
> partitions", but we don't know all the partitions in the AdminClient, yet
> we have to return a ElectPreferredLeadersResults instance, supposedly
> with
> the partitions as keys.
>
> We could handle this by passing a KafkaFuture<Map<TopicPartition,
> KafkaFuture<Void>>> to the ctor of ElectPreferredLeadersResults, instead
> of
> an extant Map<TopicPartition, KafkaFuture<Void>> (the API of
> ElectPreferredLeadersResults would not change). In the case that the
> partitions argument was not null this future will already be completed.
> In
> the case where partitions argument was null this future will be completed
> when we have a response from which we discover the partitions; in the
> meantime the AdminClient can carry on being used for other calls. So in
> the
> normal case there's not really a problem.
>
> The problem comes where there's an exception *before we get the
> response*,
> that means we still don't know the partitions to populate the map with.
> In
> practice this would mean that an exception could propagate out of
> ElectPreferredLeadersResults.values() rather than when the map was
> accessed
> element-wise. Since we control the API of ElectPreferredLeadersResults we
> could document that values() (and consequently all()) could throw,. We
> could even use checked exceptions, though since the exception would only
> happen in the case that the partitions argument was null that would feel
> rather heavy-handed to me.
>
> Another alternative would be to block in
> AdminClient.electPreferredLeaders()
> in the case that the partitions argument was null, and if there was an
> error let the exception propagate out of electPreferredLeaders()
> directly.
>
> Sorry about having to ask about this once people have already voted, but
> what do people think about these options?
I think we need to be very careful to keep the APIs asynchronous all the
time. Having electPreferredLeaders() or values() sometimes block might
superficially seem reasonable, but it destroys the usefulness of the API
for true async programming. Basically the async user is forced to put
the call into a thread pool in case it decides to block. And they
cannot do nice things like chain Futures.
Why does there need to be a map at all in the API? Why not just have
something like this:
ElectPreferredLeadersResults {
/**
* Get the result of the election for the given TopicPartition.
* If there was not an election triggered for the given
TopicPartition, the
* future will complete with an error.
*/
public KafkaFuture<Void> partitionResult(TopicPartition
topicPartition);
/**
* Get the topic partitions on which we attempted to trigger an
election.
* This tracks attempts, not successes. A partition will appear in
this result
* even if the election was not successfully triggered.
*/
public KafkaFuture<Collection<TopicPartition>> partitions();
/**
* Return a future which gives an error result if we fail for any
partition.
*/
public KafkaFuture<Void> all();
}
We can fill in all this information when we actually know it. In some
cases that will be later than others. But all the calls can immediately
return a KafkaFuture, not block or throw an exception.
best,
Colin
>
> Thanks,
>
> Tom
>
> On 30 August 2017 at 16:55, Tom Bentley <t....@gmail.com> wrote:
>
> > I've updated in the KIP.
> >
> > Thanks,
> >
> > Tom
> >
> > On 30 August 2017 at 16:42, Ismael Juma <is...@juma.me.uk> wrote:
> >
> >> If you agree with the change, yes, please rename. It's OK to make changes
> >> after the VOTE thread starts. In cases where some people have already
> >> voted, it's recommended to mention the changes in the VOTE thread as a
> >> heads up. Generally, we don't restart the vote unless the changes are
> >> significant.
> >>
> >> Ismael
> >>
> >> On Wed, Aug 30, 2017 at 4:26 PM, Tom Bentley <t....@gmail.com>
> >> wrote:
> >>
> >> > Hi Ismael,
> >> >
> >> > I agree that `electPreferredReplicaLeader` is a mouthful and am happy to
> >> > change it to `electPreferredLeaders`. I'd rename the correspond request
> >> and
> >> > response similarly.
> >> >
> >> > Should I rename it in the KIP now, even though I initiated a VOTE thread
> >> > yesterday?
> >> >
> >> > Cheers,
> >> >
> >> > Tom
> >> >
> >> > On 30 August 2017 at 16:01, Ismael Juma <is...@juma.me.uk> wrote:
> >> >
> >> > > Hi Tom,
> >> > >
> >> > > Thanks for the KIP, it's a useful one. I find the proposed method name
> >> > > `electPreferredReplicaLeader` a little hard to read. It seems that a
> >> > small
> >> > > change would make it clearer: `electPreferredReplicaAsLeader`. The
> >> next
> >> > > point is that this is a batch API, so it should ideally be plural like
> >> > the
> >> > > other AdminClient methods. Maybe `electPreferredReplicasAsLeaders`,
> >> but
> >> > > that's quite a mouthful. Maybe we should shorten it to
> >> > > `electPreferredLeaders`. Thoughts?
> >> > >
> >> > > Ismael
> >> > >
> >> > > On Wed, Aug 2, 2017 at 6:34 PM, Tom Bentley <t....@gmail.com>
> >> > wrote:
> >> > >
> >> > > > In a similar vein to KIP-179 I've created KIP-183 (
> >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-183+-+Change+
> >> > > > PreferredReplicaLeaderElectionCommand+to+use+AdminClient)
> >> > > > which is about deprecating the --zookeeper option to
> >> > > > kafka-preferred-replica-election.sh and replacing it with an option
> >> > > which
> >> > > > would use a new AdminClient-based API.
> >> > > >
> >> > > > As it stands the KIP is focussed on simply moving the existing
> >> > > > functionality behind the AdminClient.
> >> > > >
> >> > > > I'd be grateful for any feedback people may have on this.
> >> > > >
> >> > > > Thanks,
> >> > > >
> >> > > > Tom
> >> > > >
> >> > >
> >> >
> >>
> >
> >