Hi Rohan, I took another look at the updated wiki page and do not have any major questions. Regarding returning a plugin object v.s. configuring a plugin object, I do not have a strong opinion except that the latter seems more consistent with existing patterns. Just curious, any other motivations to go with the latter from you?
Guozhang On Thu, Nov 9, 2023 at 11:19 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 > > > >
