Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor
QuotaCallback where > > > > > > > the plugin > > > > > > > is updated when the Cluster has changed? That would allow to > > keep an > > > > > > > internal model > > > > > > > ready. Another way would be to use batching as suggested as it > > would allow > > > > > > > to amortize > > > > > > > the cost of building a model for the current request/user. > > > > > > > > > > > > > > I also wonder if using Cluster is a good idea here. With > > KIP-500, I can > > > > > > > imagine that this > > > > > > > plugin will run in the controller directly instead of running in > > the > > > > > > > AdminManager as today. > > > > > > > In this case, we could obviously continue to build that Cluster > > object but > > > > > > > we may have > > > > > > > better ways. Therefore, I wonder if having an interface to > > represent the > > > > > > > cluster may be > > > > > > > better from an evolution perspective. Have you considered this? > > > > > > > > > > > > > > Best, > > > > > > > David > > > > > > > > > > > > > > On Mon, Nov 16, 2020 at 12:10 PM Mickael Maison < > > mickael.mai...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > If I don't see additional feedback in the next few days, I'll > > start a > > > > > > > vote. > > > > > > > > Thanks > > > > > > > > > > > > > > > > On Thu, Nov 5, 2020 at 6:29 PM Mickael Maison < > > mickael.mai...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > > > I've updated the KIP to reflect the latest discussions. > > > > > > > > > > > > > > > > > > Tom, > > > > > > > > > 2) Updated > > > > > > > > > 4) I decided against switching to a "batch interface" and > > added the > > > > > > > > > reasons in the Rejected Alternatives section > > > > > > > > > > > > > > > > > > Please take a look and let me know if you have any feedback. > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > On Tue, Oct 6, 2020 at 9:43 AM Mickael Maison < > > > > > > > mickael.mai...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > Hi Efe, > > > > > > > > > > > > > > > > > > > > Thanks for the feedback. > > > > > > > > > > We also need to assign replicas when adding partitions to > > an existing > > > > > > > > > > topic. This is why I choose to use a list of partition > > ids. Otherwise > > > > > > > > > > we'd need the number of partitions and the starting > > partition id. > > > > > > > > > > > > > > > > > > > > Let me know if you have more questions > > > > > > > > > > > > > > > > > > > > On Tue, Oct 6, 2020 at 2:16 AM Efe Gencer > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > Hi Mickael, > > > > > > > > > > > > > > > > > > > > > > Thanks for the KIP! > > > > > > > > > > > A call to an external system, e.g. Cruise Control, in the > > > > > > > > implementation of the provided interface can indeed help with > > the initial > > > > > > > > assignment of partitions. > > > > > > > > > > > > > > > > > > > > > > I am curious why the proposed > > > > > > > > `ReplicaAssignor#assignReplic
Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor
t; > > On Mon, Nov 16, 2020 at 12:10 PM Mickael Maison < > mickael.mai...@gmail.com> > > > > > > wrote: > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > If I don't see additional feedback in the next few days, I'll > start a > > > > > > vote. > > > > > > > Thanks > > > > > > > > > > > > > > On Thu, Nov 5, 2020 at 6:29 PM Mickael Maison < > mickael.mai...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > I've updated the KIP to reflect the latest discussions. > > > > > > > > > > > > > > > > Tom, > > > > > > > > 2) Updated > > > > > > > > 4) I decided against switching to a "batch interface" and > added the > > > > > > > > reasons in the Rejected Alternatives section > > > > > > > > > > > > > > > > Please take a look and let me know if you have any feedback. > > > > > > > > Thanks > > > > > > > > > > > > > > > > On Tue, Oct 6, 2020 at 9:43 AM Mickael Maison < > > > > > > mickael.mai...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > > > > Hi Efe, > > > > > > > > > > > > > > > > > > Thanks for the feedback. > > > > > > > > > We also need to assign replicas when adding partitions to > an existing > > > > > > > > > topic. This is why I choose to use a list of partition > ids. Otherwise > > > > > > > > > we'd need the number of partitions and the starting > partition id. > > > > > > > > > > > > > > > > > > Let me know if you have more questions > > > > > > > > > > > > > > > > > > On Tue, Oct 6, 2020 at 2:16 AM Efe Gencer > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > Hi Mickael, > > > > > > > > > > > > > > > > > > > > Thanks for the KIP! > > > > > > > > > > A call to an external system, e.g. Cruise Control, in the > > > > > > > implementation of the provided interface can indeed help with > the initial > > > > > > > assignment of partitions. > > > > > > > > > > > > > > > > > > > > I am curious why the proposed > > > > > > > `ReplicaAssignor#assignReplicasToBrokers` receives a list of > partition > > > > > > ids > > > > > > > as opposed to the number of partitions to create the topic > with? > > > > > > > > > > > > > > > > > > > > Would you clarify if this API is expected to be used (1) > only for > > > > > > > new topics or (2) also for existing topics? > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > > Efe > > > > > > > > > > > > > > > > > > > > From: Mickael Maison > > > > > > > > > > Sent: Thursday, October 1, 2020 9:43 AM > > > > > > > > > > To: dev > > > > > > > > > > Subject: Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor > > > > > > > > > > > > > > > > > > > > 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
Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor
t; making the logic pluggable. > > > > > > Thanks > > > > > > On Mon, Nov 16, 2020 at 3:56 PM Gwen Shapira wrote: > > > > > > > > Why would the replica placement logic run in the controller rather than > > > > in > > > > the AdminManager? > > > > > > > > My understanding, and correct me if I got it wrong, is that we are > > > > aiming > > > > at better separation of concerns. The controller job will be managing > > > > consensus and consistency of metadata, but creating the metadata itself > > > > will be in the AdminManager. > > > > > > > > On Mon, Nov 16, 2020, 5:31 AM David Jacot wrote: > > > > > > > > > Hi Mickael, > > > > > > > > > > Thanks for the KIP. It is an interesting one. > > > > > > > > > > I imagine that custom assignors may use a rather complex model of the > > > > > cluster in order > > > > > to be able to allocate partitions in smarter ways. For instance, one > > > > > may > > > > > use the distribution > > > > > of leaders in the cluster to allocate the new leaders. With the > > > > > current > > > > > interface, that > > > > > means computing the distribution based on the Cluster received for > > > > > every > > > > > assignment > > > > > request. That could become pretty inefficient in clusters with a large > > > > > number of nodes > > > > > and/or partitions. That could become even worse if the model is more > > > > > complicated. > > > > > > > > > > I wonder if you have thought about this or experienced this with your > > > > > prototype? > > > > > > > > > > Have you considered going with an approach à la ClientQuotaCallback > > > > > where > > > > > the plugin > > > > > is updated when the Cluster has changed? That would allow to keep an > > > > > internal model > > > > > ready. Another way would be to use batching as suggested as it would > > > > > allow > > > > > to amortize > > > > > the cost of building a model for the current request/user. > > > > > > > > > > I also wonder if using Cluster is a good idea here. With KIP-500, I > > > > > can > > > > > imagine that this > > > > > plugin will run in the controller directly instead of running in the > > > > > AdminManager as today. > > > > > In this case, we could obviously continue to build that Cluster > > > > > object but > > > > > we may have > > > > > better ways. Therefore, I wonder if having an interface to represent > > > > > the > > > > > cluster may be > > > > > better from an evolution perspective. Have you considered this? > > > > > > > > > > Best, > > > > > David > > > > > > > > > > On Mon, Nov 16, 2020 at 12:10 PM Mickael Maison > > > > > > > > > > wrote: > > > > > > > > > > > Hi all, > > > > > > > > > > > > If I don't see additional feedback in the next few days, I'll start > > > > > > a > > > > > vote. > > > > > > Thanks > > > > > > > > > > > > On Thu, Nov 5, 2020 at 6:29 PM Mickael Maison > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > I've updated the KIP to reflect the latest discussions. > > > > > > > > > > > > > > Tom, > > > > > > > 2) Updated > > > > > > > 4) I decided against switching to a "batch interface" and added > > > > > > > the > > > > > > > reasons in the Rejected Alternatives section > > > > > > > > > > > > > > Please take a look and let me know if you have any feedback. > > > > > > > Thanks > > > > > > > > > > > > > > On Tue, Oct 6, 2020 at 9:43 AM Mickael Maison < > > > > > mickael.mai...@gmail.com> > > > > > > wrote: > > > >
Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor
gt; > > > > > I also wonder if using Cluster is a good idea here. With KIP-500, I can > > > > imagine that this > > > > plugin will run in the controller directly instead of running in the > > > > AdminManager as today. > > > > In this case, we could obviously continue to build that Cluster object > > > > but > > > > we may have > > > > better ways. Therefore, I wonder if having an interface to represent the > > > > cluster may be > > > > better from an evolution perspective. Have you considered this? > > > > > > > > Best, > > > > David > > > > > > > > On Mon, Nov 16, 2020 at 12:10 PM Mickael Maison > > > > > > > > wrote: > > > > > > > > > Hi all, > > > > > > > > > > If I don't see additional feedback in the next few days, I'll start a > > > > vote. > > > > > Thanks > > > > > > > > > > On Thu, Nov 5, 2020 at 6:29 PM Mickael Maison > > > > > > > > > > wrote: > > > > > > > > > > > > Hi all, > > > > > > > > > > > > I've updated the KIP to reflect the latest discussions. > > > > > > > > > > > > Tom, > > > > > > 2) Updated > > > > > > 4) I decided against switching to a "batch interface" and added the > > > > > > reasons in the Rejected Alternatives section > > > > > > > > > > > > Please take a look and let me know if you have any feedback. > > > > > > Thanks > > > > > > > > > > > > On Tue, Oct 6, 2020 at 9:43 AM Mickael Maison < > > > > mickael.mai...@gmail.com> > > > > > wrote: > > > > > > > > > > > > > > Hi Efe, > > > > > > > > > > > > > > Thanks for the feedback. > > > > > > > We also need to assign replicas when adding partitions to an > > > > > > > existing > > > > > > > topic. This is why I choose to use a list of partition ids. > > > > > > > Otherwise > > > > > > > we'd need the number of partitions and the starting partition id. > > > > > > > > > > > > > > Let me know if you have more questions > > > > > > > > > > > > > > On Tue, Oct 6, 2020 at 2:16 AM Efe Gencer > > > > > > > > > wrote: > > > > > > > > > > > > > > > > Hi Mickael, > > > > > > > > > > > > > > > > Thanks for the KIP! > > > > > > > > A call to an external system, e.g. Cruise Control, in the > > > > > implementation of the provided interface can indeed help with the > > > > > initial > > > > > assignment of partitions. > > > > > > > > > > > > > > > > I am curious why the proposed > > > > > `ReplicaAssignor#assignReplicasToBrokers` receives a list of partition > > > > ids > > > > > as opposed to the number of partitions to create the topic with? > > > > > > > > > > > > > > > > Would you clarify if this API is expected to be used (1) only > > > > > > > > for > > > > > new topics or (2) also for existing topics? > > > > > > > > > > > > > > > > Best, > > > > > > > > Efe > > > > > > > > > > > > > > > > From: Mickael Maison > > > > > > > > Sent: Thursday, October 1, 2020 9:43 AM > > > > > > > > To: dev > > > > > > > > Subject: Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor > > > > > > > > > > > > > > > > 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 > > > > > > >
Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor
; > 4) I decided against switching to a "batch interface" and added the > > > > > reasons in the Rejected Alternatives section > > > > > > > > > > Please take a look and let me know if you have any feedback. > > > > > Thanks > > > > > > > > > > On Tue, Oct 6, 2020 at 9:43 AM Mickael Maison < > > > mickael.mai...@gmail.com> > > > > wrote: > > > > > > > > > > > > Hi Efe, > > > > > > > > > > > > Thanks for the feedback. > > > > > > We also need to assign replicas when adding partitions to an > > > > > > existing > > > > > > topic. This is why I choose to use a list of partition ids. > > > > > > Otherwise > > > > > > we'd need the number of partitions and the starting partition id. > > > > > > > > > > > > Let me know if you have more questions > > > > > > > > > > > > On Tue, Oct 6, 2020 at 2:16 AM Efe Gencer > > > > > > > wrote: > > > > > > > > > > > > > > Hi Mickael, > > > > > > > > > > > > > > Thanks for the KIP! > > > > > > > A call to an external system, e.g. Cruise Control, in the > > > > implementation of the provided interface can indeed help with the > > > > initial > > > > assignment of partitions. > > > > > > > > > > > > > > I am curious why the proposed > > > > `ReplicaAssignor#assignReplicasToBrokers` receives a list of partition > > > ids > > > > as opposed to the number of partitions to create the topic with? > > > > > > > > > > > > > > Would you clarify if this API is expected to be used (1) only for > > > > new topics or (2) also for existing topics? > > > > > > > > > > > > > > Best, > > > > > > > Efe > > > > > > > > > > > > > > From: Mickael Maison > > > > > > > Sent: Thursday, October 1, 2020 9:43 AM > > > > > > > To: dev > > > > > > > Subject: Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor > > > > > > > > > > > > > > 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 > > > > 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? > > > >
Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor
Hi David, I think using updateClusterMetadata() like in ClientQuotaCallback is a great idea. I've updated the KIP accordingly. As mentioned, computing assignments for the full batch of topics/partitions was considered but it made the logic in AdminManager really complicated. For the initial use cases this KIP is targetting, it felt simpler and acceptable to compute assignments one topic at a time. Cluster is already used in other APIs, like ClientQuotaCallback so I think it makes sense to reuse it here. I'm not fully up to date with the latest advances on KIP-500, but like Gwen, I'm not sure we'd want to move that logic into the Controller. This KIP is keeping the metadata creation in AdminManager and just making the logic pluggable. Thanks On Mon, Nov 16, 2020 at 3:56 PM Gwen Shapira wrote: > > Why would the replica placement logic run in the controller rather than in > the AdminManager? > > My understanding, and correct me if I got it wrong, is that we are aiming > at better separation of concerns. The controller job will be managing > consensus and consistency of metadata, but creating the metadata itself > will be in the AdminManager. > > On Mon, Nov 16, 2020, 5:31 AM David Jacot wrote: > > > Hi Mickael, > > > > Thanks for the KIP. It is an interesting one. > > > > I imagine that custom assignors may use a rather complex model of the > > cluster in order > > to be able to allocate partitions in smarter ways. For instance, one may > > use the distribution > > of leaders in the cluster to allocate the new leaders. With the current > > interface, that > > means computing the distribution based on the Cluster received for every > > assignment > > request. That could become pretty inefficient in clusters with a large > > number of nodes > > and/or partitions. That could become even worse if the model is more > > complicated. > > > > I wonder if you have thought about this or experienced this with your > > prototype? > > > > Have you considered going with an approach à la ClientQuotaCallback where > > the plugin > > is updated when the Cluster has changed? That would allow to keep an > > internal model > > ready. Another way would be to use batching as suggested as it would allow > > to amortize > > the cost of building a model for the current request/user. > > > > I also wonder if using Cluster is a good idea here. With KIP-500, I can > > imagine that this > > plugin will run in the controller directly instead of running in the > > AdminManager as today. > > In this case, we could obviously continue to build that Cluster object but > > we may have > > better ways. Therefore, I wonder if having an interface to represent the > > cluster may be > > better from an evolution perspective. Have you considered this? > > > > Best, > > David > > > > On Mon, Nov 16, 2020 at 12:10 PM Mickael Maison > > wrote: > > > > > Hi all, > > > > > > If I don't see additional feedback in the next few days, I'll start a > > vote. > > > Thanks > > > > > > On Thu, Nov 5, 2020 at 6:29 PM Mickael Maison > > > wrote: > > > > > > > > Hi all, > > > > > > > > I've updated the KIP to reflect the latest discussions. > > > > > > > > Tom, > > > > 2) Updated > > > > 4) I decided against switching to a "batch interface" and added the > > > > reasons in the Rejected Alternatives section > > > > > > > > Please take a look and let me know if you have any feedback. > > > > Thanks > > > > > > > > On Tue, Oct 6, 2020 at 9:43 AM Mickael Maison < > > mickael.mai...@gmail.com> > > > wrote: > > > > > > > > > > Hi Efe, > > > > > > > > > > Thanks for the feedback. > > > > > We also need to assign replicas when adding partitions to an existing > > > > > topic. This is why I choose to use a list of partition ids. Otherwise > > > > > we'd need the number of partitions and the starting partition id. > > > > > > > > > > Let me know if you have more questions > > > > > > > > > > On Tue, Oct 6, 2020 at 2:16 AM Efe Gencer > > > > > wrote: > > > > > > > > > > > > Hi Mickael, > > > > > > > > > > > > Thanks for the KIP! > > > > > > A call to an external system, e.g. Cruise Control, in the > > > implementation of the p
Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor
Why would the replica placement logic run in the controller rather than in the AdminManager? My understanding, and correct me if I got it wrong, is that we are aiming at better separation of concerns. The controller job will be managing consensus and consistency of metadata, but creating the metadata itself will be in the AdminManager. On Mon, Nov 16, 2020, 5:31 AM David Jacot wrote: > Hi Mickael, > > Thanks for the KIP. It is an interesting one. > > I imagine that custom assignors may use a rather complex model of the > cluster in order > to be able to allocate partitions in smarter ways. For instance, one may > use the distribution > of leaders in the cluster to allocate the new leaders. With the current > interface, that > means computing the distribution based on the Cluster received for every > assignment > request. That could become pretty inefficient in clusters with a large > number of nodes > and/or partitions. That could become even worse if the model is more > complicated. > > I wonder if you have thought about this or experienced this with your > prototype? > > Have you considered going with an approach à la ClientQuotaCallback where > the plugin > is updated when the Cluster has changed? That would allow to keep an > internal model > ready. Another way would be to use batching as suggested as it would allow > to amortize > the cost of building a model for the current request/user. > > I also wonder if using Cluster is a good idea here. With KIP-500, I can > imagine that this > plugin will run in the controller directly instead of running in the > AdminManager as today. > In this case, we could obviously continue to build that Cluster object but > we may have > better ways. Therefore, I wonder if having an interface to represent the > cluster may be > better from an evolution perspective. Have you considered this? > > Best, > David > > On Mon, Nov 16, 2020 at 12:10 PM Mickael Maison > wrote: > > > Hi all, > > > > If I don't see additional feedback in the next few days, I'll start a > vote. > > Thanks > > > > On Thu, Nov 5, 2020 at 6:29 PM Mickael Maison > > wrote: > > > > > > Hi all, > > > > > > I've updated the KIP to reflect the latest discussions. > > > > > > Tom, > > > 2) Updated > > > 4) I decided against switching to a "batch interface" and added the > > > reasons in the Rejected Alternatives section > > > > > > Please take a look and let me know if you have any feedback. > > > Thanks > > > > > > On Tue, Oct 6, 2020 at 9:43 AM Mickael Maison < > mickael.mai...@gmail.com> > > wrote: > > > > > > > > Hi Efe, > > > > > > > > Thanks for the feedback. > > > > We also need to assign replicas when adding partitions to an existing > > > > topic. This is why I choose to use a list of partition ids. Otherwise > > > > we'd need the number of partitions and the starting partition id. > > > > > > > > Let me know if you have more questions > > > > > > > > On Tue, Oct 6, 2020 at 2:16 AM Efe Gencer > > > wrote: > > > > > > > > > > Hi Mickael, > > > > > > > > > > Thanks for the KIP! > > > > > A call to an external system, e.g. Cruise Control, in the > > implementation of the provided interface can indeed help with the initial > > assignment of partitions. > > > > > > > > > > I am curious why the proposed > > `ReplicaAssignor#assignReplicasToBrokers` receives a list of partition > ids > > as opposed to the number of partitions to create the topic with? > > > > > > > > > > Would you clarify if this API is expected to be used (1) only for > > new topics or (2) also for existing topics? > > > > > > > > > > Best, > > > > > Efe > > > > > > > > > > From: Mickael Maison > > > > > Sent: Thursday, October 1, 2020 9:43 AM > > > > > To: dev > > > > > Subject: Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor > > > > > > > > > > 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 > > >
Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor
Hi Mickael, Thanks for the KIP. It is an interesting one. I imagine that custom assignors may use a rather complex model of the cluster in order to be able to allocate partitions in smarter ways. For instance, one may use the distribution of leaders in the cluster to allocate the new leaders. With the current interface, that means computing the distribution based on the Cluster received for every assignment request. That could become pretty inefficient in clusters with a large number of nodes and/or partitions. That could become even worse if the model is more complicated. I wonder if you have thought about this or experienced this with your prototype? Have you considered going with an approach à la ClientQuotaCallback where the plugin is updated when the Cluster has changed? That would allow to keep an internal model ready. Another way would be to use batching as suggested as it would allow to amortize the cost of building a model for the current request/user. I also wonder if using Cluster is a good idea here. With KIP-500, I can imagine that this plugin will run in the controller directly instead of running in the AdminManager as today. In this case, we could obviously continue to build that Cluster object but we may have better ways. Therefore, I wonder if having an interface to represent the cluster may be better from an evolution perspective. Have you considered this? Best, David On Mon, Nov 16, 2020 at 12:10 PM Mickael Maison wrote: > Hi all, > > If I don't see additional feedback in the next few days, I'll start a vote. > Thanks > > On Thu, Nov 5, 2020 at 6:29 PM Mickael Maison > wrote: > > > > Hi all, > > > > I've updated the KIP to reflect the latest discussions. > > > > Tom, > > 2) Updated > > 4) I decided against switching to a "batch interface" and added the > > reasons in the Rejected Alternatives section > > > > Please take a look and let me know if you have any feedback. > > Thanks > > > > On Tue, Oct 6, 2020 at 9:43 AM Mickael Maison > wrote: > > > > > > Hi Efe, > > > > > > Thanks for the feedback. > > > We also need to assign replicas when adding partitions to an existing > > > topic. This is why I choose to use a list of partition ids. Otherwise > > > we'd need the number of partitions and the starting partition id. > > > > > > Let me know if you have more questions > > > > > > On Tue, Oct 6, 2020 at 2:16 AM Efe Gencer > wrote: > > > > > > > > Hi Mickael, > > > > > > > > Thanks for the KIP! > > > > A call to an external system, e.g. Cruise Control, in the > implementation of the provided interface can indeed help with the initial > assignment of partitions. > > > > > > > > I am curious why the proposed > `ReplicaAssignor#assignReplicasToBrokers` receives a list of partition ids > as opposed to the number of partitions to create the topic with? > > > > > > > > Would you clarify if this API is expected to be used (1) only for > new topics or (2) also for existing topics? > > > > > > > > Best, > > > > Efe > > > > > > > > From: Mickael Maison > > > > Sent: Thursday, October 1, 2020 9:43 AM > > > > To: dev > > > > Subject: Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor > > > > > > > > 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 > wrote: > > > >
Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor
Hi all, If I don't see additional feedback in the next few days, I'll start a vote. Thanks On Thu, Nov 5, 2020 at 6:29 PM Mickael Maison wrote: > > Hi all, > > I've updated the KIP to reflect the latest discussions. > > Tom, > 2) Updated > 4) I decided against switching to a "batch interface" and added the > reasons in the Rejected Alternatives section > > Please take a look and let me know if you have any feedback. > Thanks > > On Tue, Oct 6, 2020 at 9:43 AM Mickael Maison > wrote: > > > > Hi Efe, > > > > Thanks for the feedback. > > We also need to assign replicas when adding partitions to an existing > > topic. This is why I choose to use a list of partition ids. Otherwise > > we'd need the number of partitions and the starting partition id. > > > > Let me know if you have more questions > > > > On Tue, Oct 6, 2020 at 2:16 AM Efe Gencer > > wrote: > > > > > > Hi Mickael, > > > > > > Thanks for the KIP! > > > A call to an external system, e.g. Cruise Control, in the implementation > > > of the provided interface can indeed help with the initial assignment of > > > partitions. > > > > > > I am curious why the proposed `ReplicaAssignor#assignReplicasToBrokers` > > > receives a list of partition ids as opposed to the number of partitions > > > to create the topic with? > > > > > > Would you clarify if this API is expected to be used (1) only for new > > > topics or (2) also for existing topics? > > > > > > Best, > > > Efe > > > > > > From: Mickael Maison > > > Sent: Thursday, October 1, 2020 9:43 AM > > > To: dev > > > Subject: Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor > > > > > > 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 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 > > >
Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor
Hi all, I've updated the KIP to reflect the latest discussions. Tom, 2) Updated 4) I decided against switching to a "batch interface" and added the reasons in the Rejected Alternatives section Please take a look and let me know if you have any feedback. Thanks On Tue, Oct 6, 2020 at 9:43 AM Mickael Maison wrote: > > Hi Efe, > > Thanks for the feedback. > We also need to assign replicas when adding partitions to an existing > topic. This is why I choose to use a list of partition ids. Otherwise > we'd need the number of partitions and the starting partition id. > > Let me know if you have more questions > > On Tue, Oct 6, 2020 at 2:16 AM Efe Gencer > wrote: > > > > Hi Mickael, > > > > Thanks for the KIP! > > A call to an external system, e.g. Cruise Control, in the implementation of > > the provided interface can indeed help with the initial assignment of > > partitions. > > > > I am curious why the proposed `ReplicaAssignor#assignReplicasToBrokers` > > receives a list of partition ids as opposed to the number of partitions to > > create the topic with? > > > > Would you clarify if this API is expected to be used (1) only for new > > topics or (2) also for existing topics? > > > > Best, > > Efe > > ________ > > From: Mickael Maison > > Sent: Thursday, October 1, 2020 9:43 AM > > To: dev > > Subject: Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor > > > > 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 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
Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor
Hi Efe, Thanks for the feedback. We also need to assign replicas when adding partitions to an existing topic. This is why I choose to use a list of partition ids. Otherwise we'd need the number of partitions and the starting partition id. Let me know if you have more questions On Tue, Oct 6, 2020 at 2:16 AM Efe Gencer wrote: > > Hi Mickael, > > Thanks for the KIP! > A call to an external system, e.g. Cruise Control, in the implementation of > the provided interface can indeed help with the initial assignment of > partitions. > > I am curious why the proposed `ReplicaAssignor#assignReplicasToBrokers` > receives a list of partition ids as opposed to the number of partitions to > create the topic with? > > Would you clarify if this API is expected to be used (1) only for new topics > or (2) also for existing topics? > > Best, > Efe > > From: Mickael Maison > Sent: Thursday, October 1, 2020 9:43 AM > To: dev > Subject: Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor > > 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 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 > > wrote: > > > > > Thanks Mickael, I think adding the new Exception resolves my concerns. > > > > > > On Thu, Sep 3, 2020 at 9:47 AM Mickael Maison > > > wrote: > > > > > > > Thanks Robert and Ryanne for the feedback. > > > > > > > > ReplicaAssignor implementations can throw an exception to indicate an > > > > assignment can't be computed. This
Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor
Hi Mickael, Thanks for the KIP! A call to an external system, e.g. Cruise Control, in the implementation of the provided interface can indeed help with the initial assignment of partitions. I am curious why the proposed `ReplicaAssignor#assignReplicasToBrokers` receives a list of partition ids as opposed to the number of partitions to create the topic with? Would you clarify if this API is expected to be used (1) only for new topics or (2) also for existing topics? Best, Efe From: Mickael Maison Sent: Thursday, October 1, 2020 9:43 AM To: dev Subject: Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor 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 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 > wrote: > > > Thanks Mickael, I think adding the new Exception resolves my concerns. > > > > On Thu, Sep 3, 2020 at 9:47 AM Mickael Maison > > 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 > > > wrote: > > > > > > > > Thanks Mickael, the KIP makes sense to me, esp for cases where an > > > external > > > > system (like cruise control or
Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor
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 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 > wrote: > > > Thanks Mickael, I think adding the new Exception resolves my concerns. > > > > On Thu, Sep 3, 2020 at 9:47 AM Mickael Maison > > 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 > > > 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 > > > > > > > > > >
Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor
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 wrote: > Thanks Mickael, I think adding the new Exception resolves my concerns. > > On Thu, Sep 3, 2020 at 9:47 AM Mickael Maison > 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 > > 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 > > > > > > >
Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor
Thanks Mickael, I think adding the new Exception resolves my concerns. On Thu, Sep 3, 2020 at 9:47 AM Mickael Maison 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 > 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 > > 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 > > > >
Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor
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 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 > 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 > >
Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor
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 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 >
Re: [DISCUSS] KIP-660: Pluggable ReplicaAssignor
Hi Mickael, Thanks for the KIP! One question I have is around failure cases. Are ReplicaAssignor implementations expected to always compute an assignment, or is it possible for them to have unsatisfiable conditions? One example I can think of is a requirement that at least one partition be placed in a specific rack, but no brokers in that rack are currently online. If it is possible for the assignor to fail, we should specify what that looks like (exception type, how it's handled, etc). If not, we should call out that the implementations are expected to always compute an assignment. Thanks, Bob On Thu, Aug 20, 2020 at 8:46 AM Mickael Maison 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 >
[DISCUSS] KIP-660: Pluggable ReplicaAssignor
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
