Hi Xintong, Thanks for starting the vote. The proposed changes look good to me.
+1 (binding) Best, Gary On Mon, Sep 23, 2019 at 4:18 PM Till Rohrmann <trohrm...@apache.org> wrote: > Thanks for updating the Flip. It looks good to me. > > +1 (binding) > > Cheers, > Till > > On Mon, Sep 23, 2019 at 4:12 PM Xintong Song <tonysong...@gmail.com> > wrote: > > > @Till @Andrey > > > > According to the comments, I just updated the FLIP document [1], with the > > following changes: > > > > - Remove SlotID (in the section Protocol Changes) > > - Updated implementation steps to reduce separated code paths. As far > as > > I can see at the moment, we do not need the feature option. We can add > > it > > if later we find it necessary in the implementation. > > > > > > Thank you~ > > > > Xintong Song > > > > > > [1] > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-56%3A+Dynamic+Slot+Allocation > > > > On Fri, Sep 20, 2019 at 11:01 AM Xintong Song <tonysong...@gmail.com> > > wrote: > > > > > I'm not sure if I understand the implementation plan you suggested > > > correctly. To my understanding, it seems that all the steps except for > > step > > > 5 have to happen in strict order. > > > > > > - Profiles to be used in step 2 is reported with step 1. > > > - SlotProfile in TaskExecutorGateway#requestSlot in step 3 comes > from > > > profiles used in step 2. > > > - Only if RM request slots from TM with profiles (step 3), would TM > be > > > able to do the proper bookkeeping (step 4) > > > - Step 5 can be done as long as we have step 2. > > > - Step 6 relies on both step 4 and step 5, for proper bookkeepings > on > > > both TM and RM sides before enabling non-default profiles. > > > > > > That means we can only work on the steps in the following order. > > > 1-2-3-4-6 > > > \-5-/ > > > > > > What I'm trying to achieve with the current plan, is to have most of > the > > > implementation steps paralleled, as the following. So that Andrey and I > > can > > > work concurrently without blocking each other too much. > > > 1-2-3-4 > > > \5-6-7 > > > > > > > > > I also agree that it would be good to not add too much separate codes. > I > > > would suggest leave that decision to the implementation time. E.g., if > by > > > the time we do the TM side bookkeeping, the RM side has already > > implemented > > > requesting slots with profiles, then we do not need to separate the > code > > > paths. > > > > > > > > > To that end, I think it makes sense to adjust step 5-7 to first use > > > default slot resource profiles for all the bookkeepings, and replace it > > > with the requested profiles at the end. > > > > > > > > > What do you think? > > > > > > > > > Thank you~ > > > > > > Xintong Song > > > > > > > > > > > > On Thu, Sep 19, 2019 at 7:59 PM Till Rohrmann <trohrm...@apache.org> > > > wrote: > > > > > >> I think besides of point 1. and 3. there are no dependencies between > the > > >> RM > > >> and TM side changes. Also, I'm not sure whether it makes sense to > split > > >> the > > >> slot manager changes up into the proposed steps 5, 6 and 7. > > >> > > >> I would highly recommend to not add too much duplicate logic/separate > > code > > >> paths because it just adds blind spots which are probably not as well > > >> tested as the old code paths. > > >> > > >> Cheers, > > >> Till > > >> > > >> On Thu, Sep 19, 2019 at 11:58 AM Xintong Song <tonysong...@gmail.com> > > >> wrote: > > >> > > >> > Thanks for the comments, Till. > > >> > > > >> > - Agree on removing SlotID. > > >> > > > >> > - Regarding the implementation plan, it is true that we can possibly > > >> reduce > > >> > codes separated by the feature option. But I think to do that we > need > > to > > >> > introduce more dependencies between implementation steps. With the > > >> current > > >> > plan, we can easily separate steps on the RM side and the TM side, > and > > >> > start concurrently working on them after quickly updating the > > >> interfaces in > > >> > between. The feature will come alive when the steps on both RM/TM > > sides > > >> are > > >> > finished. Since we are planning to have two persons (Andrey and I) > > >> working > > >> > on this FLIP, I think the current plan is probably more convenient. > > >> > > > >> > Thank you~ > > >> > > > >> > Xintong Song > > >> > > > >> > > > >> > > > >> > On Thu, Sep 19, 2019 at 5:09 PM Till Rohrmann <trohrm...@apache.org > > > > >> > wrote: > > >> > > > >> > > Hi Xintong, > > >> > > > > >> > > thanks for starting the vote. The general plan looks good. Hence > +1 > > >> from > > >> > my > > >> > > side. I still have some minor comments one could think about: > > >> > > > > >> > > * As we no longer have predetermined slots on the TaskExecutor, I > > >> think > > >> > we > > >> > > can get rid of the SlotID. Instead, an allocated slot will be > > >> identified > > >> > by > > >> > > the AllocationID and the TaskManager's ResourceID in order to > > >> > differentiate > > >> > > duplicate registrations. > > >> > > * For the implementation plan, I believe there is only one tiny > part > > >> on > > >> > the > > >> > > SlotManager for which we need a separate code path/feature flag > > which > > >> is > > >> > > how we find a matching slot. Everything else should be possible to > > >> > > implement in a way that it works with dynamic and static slot > > >> allocation: > > >> > > 1. Let TMs register with default slot profile at RM > > >> > > 2. Change SlotManager to use reported slot profiles instead of > > >> > > pre-calculated profiles > > >> > > 3. Replace SlotID with SlotProfile in > > TaskExecutorGateway#requestSlot > > >> > > 4. Extend TM to support dynamic slot allocation (aka proper > > >> bookkeeping) > > >> > > (can happen concurrently to any of steps 2-3) > > >> > > 5. Add bookkeeping to SlotManager (for pending TMs and registered > > TMs) > > >> > but > > >> > > still only use default slot profiles for matching with slot > requests > > >> > > 6. Allow to match slot requests with reported resources instead of > > >> > default > > >> > > slot profiles (here we could use a feature flag to switch between > > >> dynamic > > >> > > and static slot allocation) > > >> > > > > >> > > Wdyt? > > >> > > > > >> > > Cheers, > > >> > > Till > > >> > > > > >> > > On Thu, Sep 19, 2019 at 9:45 AM Andrey Zagrebin < > > azagre...@apache.org > > >> > > > >> > > wrote: > > >> > > > > >> > > > Hi Xintong, > > >> > > > > > >> > > > Thanks for starting the vote, +1 from my side. > > >> > > > > > >> > > > Best, > > >> > > > Andrey > > >> > > > > > >> > > > On Tue, Sep 17, 2019 at 4:26 PM Xintong Song < > > tonysong...@gmail.com > > >> > > > >> > > > wrote: > > >> > > > > > >> > > > > Hi all, > > >> > > > > > > >> > > > > I would like to start the vote for FLIP-56 [1], on which a > > >> consensus > > >> > is > > >> > > > > reached in this discussion thread [2]. > > >> > > > > > > >> > > > > The vote will be open for at least 72 hours. I'll try to close > > it > > >> > after > > >> > > > > Sep. 20 15:00 UTC, unless there is an objection or not enough > > >> votes. > > >> > > > > > > >> > > > > Thank you~ > > >> > > > > > > >> > > > > Xintong Song > > >> > > > > > > >> > > > > > > >> > > > > [1] > > >> > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-56%3A+Dynamic+Slot+Allocation > > >> > > > > > > >> > > > > [2] > > >> > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > http://apache-flink-mailing-list-archive.1008284.n3.nabble.com/DISCUSS-FLIP-56-Dynamic-Slot-Allocation-td31960.html > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > >
