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