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 > > > > > > >
