Thanks for the feedback Bruno! For the most part I think it makes sense, but leaving a couple follow-up thoughts/questions:
re 4: I think Sophie's point was slightly different - that we might want to wrap the return type for `assign` in a class so that its easily extensible. This makes sense to me. Whether we do that or not, we can have the return type be a Set instead of a Map as well. re 6: Yes, it's a callback that's called with the final assignment. I like your suggested name. On Fri, Apr 5, 2024 at 12:17 PM Rohan Desai <desai.p.ro...@gmail.com> wrote: > Thanks for the feedback Sophie! > > re1: Totally agree. The fact that it's related to the partition assignor > is clear from just `task.assignor`. I'll update. > re3: This is a good point, and something I would find useful personally. I > think its worth adding an interface that lets the plugin observe the final > assignment. I'll add that. > re4: I like the new `NodeAssignment` type. I'll update the KIP with that. > > On Thu, Nov 9, 2023 at 11:18 PM Rohan Desai <desai.p.ro...@gmail.com> > wrote: > >> Thanks for the feedback so far! I think pretty much all of it is >> reasonable. I'll reply to it inline: >> >> > 1. All the API logic is granular at the Task level, except the >> previousOwnerForPartition func. I’m not clear what’s the motivation >> behind it, does our controller also want to change how the >> partitions->tasks mapping is formed? >> You're right that this is out of place. I've removed this method as it's >> not needed by the task assignor. >> >> > 2. Just on the API layering itself: it feels a bit weird to have the >> three built-in functions (defaultStandbyTaskAssignment etc) sitting in >> the ApplicationMetadata class. If we consider them as some default util >> functions, how about introducing moving those into their own static util >> methods to separate from the ApplicationMetadata “fact objects” ? >> Agreed. Updated in the latest revision of the kip. These have been moved >> to TaskAssignorUtils >> >> > 3. I personally prefer `NodeAssignment` to be a read-only object >> containing the decisions made by the assignor, including the >> requestFollowupRebalance flag. For manipulating the half-baked results >> inside the assignor itself, maybe we can just be flexible to let users use >> whatever struts / their own classes even, if they like. WDYT? >> Agreed. Updated in the latest version of the kip. >> >> > 1. For the API, thoughts on changing the method signature to return a >> (non-Optional) TaskAssignor? Then we can either have the default >> implementation return new HighAvailabilityTaskAssignor or just have a >> default implementation class that people can extend if they don't want to >> implement every method. >> Based on some other discussion, I actually decided to get rid of the >> plugin interface, and instead use config to specify individual plugin >> behaviour. So the method you're referring to is no longer part of the >> proposal. >> >> > 3. Speaking of ApplicationMetadata, the javadoc says it's read only but >> theres methods that return void on it? It's not totally clear to me how >> that interface is supposed to be used by the assignor. It'd be nice if we >> could flip that interface such that it becomes part of the output instead >> of an input to the plugin. >> I've moved those methods to a util class. They're really utility methods >> the assignor might want to call to do some default or optimized assignment >> for some cases like rack-awareness. >> >> > 4. We should consider wrapping UUID in a ProcessID class so that we >> control >> the interface (there are a few places where UUID is directly used). >> I like it. Updated the proposal. >> >> > 5. What does NodeState#newAssignmentForNode() do? I thought the point >> was >> for the plugin to make the assignment? Is that the result of the default >> logic? >> It doesn't need to be part of the interface. I've removed it. >> >> > re 2/6: >> >> I generally agree with these points, but I'd rather hash that out in a PR >> than in the KIP review, as it'll be clearer what gets used how. It seems to >> me (committers please correct me if I'm wrong) that as long as we're on the >> same page about what information the interfaces are returning, that's ok at >> this level of discussion. >> >> On Tue, Nov 7, 2023 at 12:03 PM Rohan Desai <desai.p.ro...@gmail.com> >> wrote: >> >>> Hello All, >>> >>> I'd like to start a discussion on KIP-924 ( >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-924%3A+customizable+task+assignment+for+Streams) >>> which proposes an interface to allow users to plug into the streams >>> partition assignor. The motivation section in the KIP goes into some more >>> detail on why we think this is a useful addition. Thanks in advance for >>> your feedback! >>> >>> Best Regards, >>> >>> Rohan >>> >>>
