Hello Rohan, Thanks for the KIP! Thoughts below (seems I have similar comments to Guozhang, but I had already written this before reading his reply haha!). They're basically all minor suggestions for improvements, I wouldn't consider any of them blocking.
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. 2. Generally the KIP could benefit from reducing the method overloads. For example, we should consider generifying the NodeAssignment class so that all of the reader methods are consolidated in a single Set<AssignedTask> assignment() - an AssignedTask can have details about whether it's active/standby/stateful/stateless (this can be reused to reduce the surface area of ApplicationMetadata allTasks()/statefulTasks()). We can also probably collapse the three void return type methods in ApplicationMetadata into a single method. 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. 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). 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? 6. I wonder if all the NodeState lag information can be wrapped in a single class that dumps all the most granular information then lets users compute rollups on whatever they want instead of precomputing things like "sorted tasks by lag" or "total lag across all stores". Looking forward to this one! It opens the door to a lot of great things. Cheers, Almog On Thu, Nov 9, 2023 at 12:22 PM Guozhang Wang <guozhang.wang...@gmail.com> wrote: > Hello Rohan, > > Thanks for the KIP! Overall it looks very nice. Just some quick thoughts : > > 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? > > 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” ? > > 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? > > Thanks, > Guozhang > > On Tue, Nov 7, 2023 at 12:04 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 >
