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<TopicPartition> partitions) for the admin client. The old command allows not specifying the topic partitions which results in election for all topic partitions. How would this be expressed in the new API? Does the tool need to do a metadata request and then issue the request for all topic partitions or would there be a shortcut via the protocol (e.g. another electPreferredReplicaLeader() with no args which translates to, e.g., an empty list or null in the request)? * All the existing *Options classes for the AdminClient have at least a timeout. Should we add that to this APIs options? * Other AdminClient *Result classes provide some convenience methods when you just want all the results. Should we have that as well? * I don't think any other requests include error strings in the response, do they? I'm pretty sure we just generate the error string on the client side based on the error code. If an error code was ambiguous, we should probably just split it across 2 or more codes. * re: permissions, would there also be a per-topic permission required? (I haven't kept track of all the ACLs and required permissions so I'm not sure, but a bunch of other operations check per-resource they are modifying -- this operation could be considered to be against either Cluster or Topics.) * Should we consider making the request only valid against the broker and having the command do a metadata request to discover the controller and then send the request directly to it? I think this could a) simplify the implementation a bit by taking the ZK node out of the equation b) avoid the REPLICA_LEADER_ELECTION_IN_PROGRESS error since this is only required due to using the ZK node to communicate with the controller afaict and c) potentially open the door for a synchronous request/response in the future. -Ewen On Mon, Aug 7, 2017 at 8:21 AM, Tom Bentley <t.j.bent...@gmail.com> wrote: > 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 topic name. This would make for smaller messages when triggering > elections for multiple partitions of the same topic. > > I'd be grateful for any feedback you may have. > > Cheers, > > Tom > > On 2 August 2017 at 18:34, Tom Bentley <t.j.bent...@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 > > >
