Hi Mickael, We did discuss this earlier, and I remember not being too enthusiastic about a pluggable policy here :)
There have been several changes to the placement code in the last few weeks. (These are examples of the kind of changes that are impossible to do once an API is established, by the way.) Can you please revise the KIP to take these into account? I'd also like to understand a little bit better why we need this API when we have the explicit placement API for createTopics and createPartitions. Can you give me a few scenarios where the manual placement API would be insufficient? best, Colin On Mon, May 2, 2022, at 09:28, Mickael Maison wrote: > Hi, > > If there are no further comments, I'll start a vote in the next few days. > > Thanks, > Mickael > > On Wed, Mar 30, 2022 at 3:51 AM Luke Chen <show...@gmail.com> wrote: >> >> Hi Mickael, >> >> Thanks for the update. >> It answered my questions! >> >> Thank you. >> Luke >> >> On Wed, Mar 30, 2022 at 12:09 AM Mickael Maison <mickael.mai...@gmail.com> >> wrote: >> >> > Hi Luke, >> > >> > Thanks for the feedback. >> > >> > 1. Thanks, fixed! >> > 2. Yes that's right. It's the same behavior for topic policies >> > 3. I've added details about how the mentioned scenarios could be >> > addressed. The information required to make such decisions is not part >> > of the Kafka cluster metadata so an external input is necessary. This >> > KIP does not propose a specific mechanism for doing it. >> > >> > I hope this answers your questions. >> > >> > Thanks, >> > Mickael >> > >> > >> > On Tue, Mar 29, 2022 at 5:42 PM Mickael Maison <mickael.mai...@gmail.com> >> > wrote: >> > > >> > > Hi Ryanne, >> > > >> > > That's a good point! >> > > >> > > There's no value in having all implementations perform the same sanity >> > > checks. If the replication factor is < 1 or larger than the current >> > > number of registered brokers, the controller should directly throw >> > > InvalidReplicationFactorException and not call the ReplicaPlacer. I've >> > > updated the KIP so the place() method now only throws >> > > ReplicaPlacementException. >> > > >> > > Thanks, >> > > Mickael >> > > >> > > >> > > >> > > On Mon, Mar 28, 2022 at 6:20 PM Ryanne Dolan <ryannedo...@gmail.com> >> > wrote: >> > > > >> > > > Wondering about InvalidReplicationFactorException. Why would an >> > > > implementation throw this? Given the information passed to the method, >> > > > seems like this could only be thrown if there were obviously invalid >> > > > arguments, like a negative number or zero. Can we just guarantee such >> > > > invalid arguments aren't passed in? >> > > > >> > > > Ryanne >> > > > >> > > > On Sat, Mar 26, 2022, 8:51 AM Luke Chen <show...@gmail.com> wrote: >> > > > >> > > > > Hi Mickael, >> > > > > >> > > > > Thanks for the KIP! >> > > > > It's indeed a pain point for the Kafka admins. >> > > > > >> > > > > I have some comments: >> > > > > 1. Typo in motivation section: When administrators [when to] remove >> > brokers >> > > > > from a cluster,.... >> > > > > 2. If different `replica.placer.class.name` configs are set in all >> > > > > controllers, I think only the config for "active controller" will >> > take >> > > > > effect, right? >> > > > > 3. Could you explain more about how the proposal fixes some >> > scenarios you >> > > > > listed, ex: the new added broker case. How could we know the broker >> > is new >> > > > > added? I guess it's by checking the broker load via some metrics >> > > > > dynamically, right? >> > > > > >> > > > > >> > > > > Thank you. >> > > > > Luke >> > > > > >> > > > > On Fri, Mar 18, 2022 at 10:30 AM Ryanne Dolan <ryannedo...@gmail.com >> > > >> > > > > wrote: >> > > > > >> > > > > > Thanks Mickael, this makes sense to me! I've been wanting >> > something like >> > > > > > this in order to decommission a broker without new partitions >> > getting >> > > > > > accidentally assigned to it. >> > > > > > >> > > > > > Ryanne >> > > > > > >> > > > > > On Thu, Mar 17, 2022, 5:56 AM Mickael Maison < >> > mickael.mai...@gmail.com> >> > > > > > wrote: >> > > > > > >> > > > > > > Hi, >> > > > > > > >> > > > > > > I'd like to start a new discussion on KIP-660. I originally >> > wrote this >> > > > > > > KIP in 2020 and the initial discussion >> > > > > > > ( >> > https://lists.apache.org/thread/xn7xyb74nyt281brto4x28r9rzxm4lp9) >> > > > > > > raised some concerns especially around KRaft (which did not >> > exist at >> > > > > > > that time) and scalability. >> > > > > > > >> > > > > > > Since then, we got a new KRaft controller so I've been able to >> > revisit >> > > > > > > this KIP. I kept the KIP number as it's essentially the same >> > idea, but >> > > > > > > the proposal is significantly different: >> > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-660%3A+Pluggable+ReplicaPlacer >> > > > > > > >> > > > > > > Please take a look and let me know if you have any feedback. >> > > > > > > >> > > > > > > Thanks, >> > > > > > > Mickael >> > > > > > > >> > > > > > >> > > > > >> >
