Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-09-08 Thread Tom Bentley
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 wrote: > Hi, Tom, > > It seems that it's useful to know whether the leader is balanced for each > pa

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-09-07 Thread Jun Rao
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 wrote: > On Wed, Sep 6, 2017, at 01:18, Tom Bentley wrote: > > Hi Colin, > > >

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-09-06 Thread Colin McCabe
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 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

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-09-06 Thread Tom Bentley
Hi Colin, Thanks for taking the time to respond. On 5 September 2017 at 22:22, Colin McCabe 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 *Re

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-09-05 Thread Colin McCabe
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

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-09-04 Thread Tom Bentley
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 a

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-08-30 Thread Tom Bentley
I've updated in the KIP. Thanks, Tom On 30 August 2017 at 16:42, Ismael Juma 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 t

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-08-30 Thread Ismael Juma
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.

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-08-30 Thread Tom Bentley
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

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-08-30 Thread Ismael Juma
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 t

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-08-23 Thread Jun Rao
H, Collin, That's a good point. I agree that Alter on Cluster resource is more appropriate. Thanks, Jun On Tue, Aug 22, 2017 at 1:42 PM, Colin McCabe wrote: > Hi, > > Thanks for the KIP. It looks good overall. > > On Tue, Aug 22, 2017, at 08:54, Tom Bentley wrote: > > Hi Jun, > > > > Thanks

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-08-23 Thread Tom Bentley
Hi Colin, Thanks for your input. A couple of comments inline. On 22 August 2017 at 21:42, Colin McCabe wrote: > Hi, > > Thanks for the KIP. It looks good overall. > > On Tue, Aug 22, 2017, at 08:54, Tom Bentley wrote: > > Hi Jun, > > > > Thanks for the feedback. > > > > I've updated the KIP to

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-08-22 Thread Colin McCabe
Hi, Thanks for the KIP. It looks good overall. On Tue, Aug 22, 2017, at 08:54, Tom Bentley wrote: > Hi Jun, > > Thanks for the feedback. > > I've updated the KIP to mention this new algorithm, which I agree will be > much better from the AdminClient PoV. > > I've also reverted the authorizati

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-08-22 Thread Tom Bentley
Hi Jun, Thanks for the feedback. I've updated the KIP to mention this new algorithm, which I agree will be much better from the AdminClient PoV. I've also reverted the authorization to be against the cluster resource, rather than the topic, since, on re-reading, Ewen wasn't particularly advocati

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-08-11 Thread Jun Rao
Hi, Tom, 2. Yes, that's how manual preferred leader election currently works. I think we could use this opportunity to simplify the process a bit though. Doing preferred leader election is a lot cheaper than partition reassignment since it only involves writing the new leader to ZK. So, we probabl

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-08-11 Thread Tom Bentley
Hi Jun, Thanks for your reply, I've got a few comment inline... On 11 August 2017 at 01:51, Jun Rao wrote: > Hi, Tom, > > Thanks for the KIP. Looks good overall. A few minor comments. > > 1. In most requests with topic partitions, we nest partitions inside topic. > So, instead of [topic partiti

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-08-10 Thread Jun Rao
Hi, Tom, Thanks for the KIP. Looks good overall. A few minor comments. 1. In most requests with topic partitions, we nest partitions inside topic. So, instead of [topic partition_id], we do [topic, [partition_id]] to save some space. 2. The preferred leader election just tries to move the leader

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-08-09 Thread Ismael Juma
On Wed, Aug 9, 2017 at 11:40 AM, Tom Bentley wrote: > > There are responses with detailed error messages as well as the codes, > CreateTopicsResponse, {Describe|Alter}ConfigsResponse, and the responses > for managing ACLs for instance. To be honest, I assumed including a message > was the norm. In

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-08-09 Thread Tom Bentley
Hi Ewen, Thanks for looking at the KIP. I've updated it for some of your comments, but see also a few replies inline. On 9 August 2017 at 06:02, Ewen Cheslack-Postava wrote: > Thanks for the KIP. Generally the move away from ZK and to native Kafka > requests is good, so I'm generally +1 on this

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-08-08 Thread Ewen Cheslack-Postava
Thanks for the KIP. Generally the move away from ZK and to native Kafka requests is good, so I'm generally +1 on this. A couple of comments/questions. * You gave the signature electPreferredReplicaLeader(Collection partitions) for the admin client. The old command allows not specifying the topic p

Re: [DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-08-07 Thread Tom Bentley
Hi, I've updated this KIP slightly, to clarify a couple of points. One thing in particular that I would like feedback on is what authorization should be required for triggering the election of the preferred leader? Another thing would be whether the request and responses should be grouped by top

[DISCUSS] KIP-183 - Change PreferredReplicaLeaderElectionCommand to use AdminClient

2017-08-02 Thread Tom Bentley
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 w