Nice proposal. I can definitely see the value add it could bring based on varied custom use cases.
1. When exposing an interface external to Kafka, could we strive to ensure that the implementation can not mutate the incoming parameters? This would ensure that the implementations don't un-intentionally change some internal state of the cluster. Also, this would be a two way door, so later if we decide to relax the constraint to allow implementations to mutate certain things, we should be able to do that. This immutability can be achieved for example by passing a clone of the list of usable brokers to the interface. 2. Why is the interface Closeable? What resource does the interface expect to close? Please add a comment on the class documentation about this. 3. The iterator that is being passed as an argument to `place()`. Do we expect the place() method to mutate the input? If not, then an iterator may not be necessary? Could it be an unmodifiable collection instead? 4. Please add documentation on behaviour expected out of the place() function such as, should it be idempotent? Is it called in some latency sensitive path? What should be the performance expectation from this function? etc. 5. Please add documentation on behaviour expected out of the place() function for error scenario's e.g. would it be able to set a message in the ReplicaPlacementException? 6. The interface should be extensible in the future, e.g. if we want to add a new parameter to the place() function in future, we shouldn't have to require code changes to all concrete implementations. This can be achieved by grouping the parameters of place() into higher abstractions, say, PlacementSpec (which already exist). The advantage is that a new parameter could be added to PlacementSpec with a setter/getter in future but it wouldn't require any code modification to the custom implementations of ReplicaPlacer. Divij Vaidya On Mon, May 2, 2022 at 6:28 PM Mickael Maison <mickael.mai...@gmail.com> 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 > > > > > > > > > > > > > > > > > > > > > > > > >
