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.j.bent...@gmail.com> wrote: > I've updated in the KIP. > > Thanks, > > Tom > > On 30 August 2017 at 16:42, Ismael Juma <ism...@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.j.bent...@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 <ism...@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.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 >> > > > >> > > >> > >> > >
