Hi Weihua, Thanks for your proposal. From a conceptual point: AFAIU, the DeclarativeSlotManager covers a subset (i.e. only evenly sized slots) of what the FineGrainedSlotManager should be able to achieve (variable slot size per task manager). Is this the right assumption/understanding? In this sense, merging both implementations into a single one sounds good. A few more general comments, though:
1. Did you do a proper test coverage analysis? That's not mentioned in the current version of the FLIP. I'm bringing this up because we ran into the same issue when fixing the flaws that popped up after introducing the multi-component leader election (see FLIP-285 [1]). There is a risk that by removing the legacy code we decrease test coverage because certain test cases that were covered for the legacy classes might not be necessarily covered in the new implementation, yet (see FLINK-30338 [2] which covers this issue for the leader election case). Ideally, we don't want to remove test cases accidentally because they were only implemented for the DeclarativeSlotManager but missed for the FineGrainedSlotManager. 2. DeclarativeSlotManager and FineGrainedSlotManager feel quite big in terms of lines of code. Without knowing whether it's actually a reasonable thing to do: Instead of just adding more features to the FineGrainedSlotManager, have you thought of cutting out functionality into smaller sub-components along this refactoring? Such a step-by-step approach might improve the overall codebase and might make reviewing the refactoring easier. I did a first pass over the code and struggled to identify code blocks that could be moved out of the SlotManager implementation(s). Therefore, I might be wrong with this proposal. I haven't worked on this codebase in that detail that it would allow me to come up with a judgement call. I wanted to bring it up, anyway, because I'm curious whether that could be an option. There's a comment created by Chesnay (CC'd) in the JavaDoc of TaskExecutorManager [3] indicating something similar. I'm wondering whether he can add some insights here. 3. For me personally, having a more detailed summary comparing the subcomponents of both SlotManager implementations with where their functionality matches and where they differ might help understand the consequences of the changes proposed in FLIP-298. Best, Matthias [1] https://cwiki.apache.org/confluence/display/FLINK/FLIP-285%3A+Refactoring+LeaderElection+to+make+Flink+support+multi-component+leader+election+out-of-the-box [2] https://issues.apache.org/jira/browse/FLINK-30338 [3] https://github.com/apache/flink/blob/f611ea8cb5deddb42429df2c99f0c68d7382e9bd/flink-runtime/src/main/java/org/apache/flink/runtime/resourcemanager/slotmanager/TaskExecutorManager.java#L66-L68 On Tue, Feb 28, 2023 at 6:14 AM Matt Wang <wang...@163.com> wrote: > This is a good proposal for me, it will make the code of the SlotManager > more clear. > > > > -- > > Best, > Matt Wang > > > ---- Replied Message ---- > | From | David Morávek<d...@apache.org> | > | Date | 02/27/2023 22:45 | > | To | <dev@flink.apache.org> | > | Subject | Re: [DISCUSS] FLIP-298: Unifying the Implementation of > SlotManager | > Hi Weihua, I still need to dig into the details, but the overall sentiment > of this change sounds reasonable. > > Best, > D. > > On Mon, Feb 27, 2023 at 2:26 PM Zhanghao Chen <zhanghao.c...@outlook.com> > wrote: > > Thanks for driving this topic. I think this FLIP could help clean up the > codebase to make it easier to maintain. +1 on it. > > Best, > Zhanghao Chen > ________________________________ > From: Weihua Hu <huweihua....@gmail.com> > Sent: Monday, February 27, 2023 20:40 > To: dev <dev@flink.apache.org> > Subject: [DISCUSS] FLIP-298: Unifying the Implementation of SlotManager > > Hi everyone, > > I would like to begin a discussion on FLIP-298: Unifying the Implementation > of SlotManager[1]. There are currently two types of SlotManager in Flink: > DeclarativeSlotManager and FineGrainedSlotManager. FineGrainedSlotManager > should behave as DeclarativeSlotManager if the user does not configure the > slot request profile. > > Therefore, this FLIP aims to unify the implementation of SlotManager in > order to reduce maintenance costs. > > Looking forward to hearing from you. > > [1] > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-298%3A+Unifying+the+Implementation+of+SlotManager > > Best, > Weihua > >
