Thanks Tom for the feedback! 1. If the data returned by the ReplicaAssignor implementation does not match that was requested, we'll also throw a ReplicaAssignorException
2. Good point, I'll update the KIP 3. The KIP mentions an error code associated with ReplicaAssignorException: REPLICA_ASSIGNOR_FAILED 4. (I'm naming your last question 4.) I spent some time looking at it. Initially I wanted to follow the model from the topic policies. But as you said, computing assignments for the whole batch may be more desirable and also avoids incrementally updating the cluster state. The logic in AdminManager is very much centered around doing 1 topic at a time but as far as I can tell we should be able to update it to compute assignments for the whole batch. I'll play a bit more with 4. and I'll update the KIP in the next few days On Mon, Sep 21, 2020 at 10:29 AM Tom Bentley <tbent...@redhat.com> wrote: > > Hi Mickael, > > A few thoughts about the ReplicaAssignor contract: > > 1. What happens if a ReplicaAssignor impl returns a Map where some > assignments don't meet the given replication factor? > 2. Fixing the signature of assignReplicasToBrokers() as you have would make > it hard to pass extra information in the future (e.g. maybe someone comes > up with a use case where passing the clientId would be needed) because it > would require the interface be changed. If you factored all the parameters > into some new type then the signature could be > assignReplicasToBrokers(RequiredReplicaAssignment) and adding any new > properties to RequiredReplicaAssignment wouldn't break the contract. > 3. When an assignor throws RepliacAssignorException what error code will be > returned to the client? > > Also, this sentence got me thinking: > > > If multiple topics are present in the request, AdminManager will update > the Cluster object so the ReplicaAssignor class has access to the up to > date cluster metadata. > > Previously I've looked at how we can improve Kafka's pluggable policy > support to pass the more of the cluster state to policy implementations. A > similar problem exists there, but the more cluster state you pass the > harder it is to incrementally change it as you iterate through the topics > to be created/modified. This likely isn't a problem here and now, but it > could limit any future changes to the pluggable assignors. Did you consider > the alternative of the assignor just being passed a Set of assignments? > That means you can just pass the cluster state as it exists at the time. It > also gives the implementation more information to work with to find more > optimal assignments. For example, it could perform a bin packing type > assignment which found a better optimum for the whole collection of topics > than one which was only told about all the topics in the request > sequentially. > > Otherwise this looks like a valuable feature to me. > > Kind regards, > > Tom > > > > > > On Fri, Sep 11, 2020 at 6:19 PM Robert Barrett <bob.barr...@confluent.io> > wrote: > > > Thanks Mickael, I think adding the new Exception resolves my concerns. > > > > On Thu, Sep 3, 2020 at 9:47 AM Mickael Maison <mickael.mai...@gmail.com> > > wrote: > > > > > Thanks Robert and Ryanne for the feedback. > > > > > > ReplicaAssignor implementations can throw an exception to indicate an > > > assignment can't be computed. This is already what the current round > > > robin assignor does. Unfortunately at the moment, there are no generic > > > error codes if it fails, it's either INVALID_PARTITIONS, > > > INVALID_REPLICATION_FACTOR or worse UNKNOWN_SERVER_ERROR. > > > > > > So I think it would be nice to introduce a new Exception/Error code to > > > cover any failures in the assignor and avoid UNKNOWN_SERVER_ERROR. > > > > > > I've updated the KIP accordingly, let me know if you have more questions. > > > > > > On Fri, Aug 28, 2020 at 4:49 PM Ryanne Dolan <ryannedo...@gmail.com> > > > wrote: > > > > > > > > Thanks Mickael, the KIP makes sense to me, esp for cases where an > > > external > > > > system (like cruise control or an operator) knows more about the target > > > > cluster state than the broker does. > > > > > > > > Ryanne > > > > > > > > On Thu, Aug 20, 2020, 10:46 AM Mickael Maison < > > mickael.mai...@gmail.com> > > > > wrote: > > > > > > > > > Hi, > > > > > > > > > > I've created KIP-660 to make the replica assignment logic pluggable. > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-660%3A+Pluggable+ReplicaAssignor > > > > > > > > > > Please take a look and let me know if you have any feedback. > > > > > > > > > > Thanks > > > > > > > > > >
