Hi there,
I've updated the FLIP accordingly. Please take a look. If you have any
further concerns please let me know.
Best,
Yangze Guo
On Fri, Apr 10, 2020 at 6:40 PM Yangze Guo <karma...@gmail.com> wrote:
>
> Thanks for the feedback, Xintong.
>
> - Should we have a factory interface for `ExternalResourceDriver`, that
> takes the configuration and returns a driver instance? Otherwise, if we are
> creating the driver instance with reflection, we kind of implicitly
> requires the driver to have a public non-argument constructor. If we
> decided to go with this approach, then we will not need
> `ExternalResourceDriver#open`.
>
> True, we could have an `ExternalResourceDriverFactory`, like
> interface ExternalResourceDriverFactory {
> ExternalResourceDriver fromConfiguration(Configuration config);
> }
> Regarding the configuration, the user should provide
> "external-resource.{resourceName}.driver-factory.class" instead.
>
> - Not sure about the necessity of `ExternalResourceDriver#close`. I would
> suggest to avoid introduce more interfaces if not absolutely necessary.
>
> I add `ExternalResourceDriver#close` in case user needs to clean up
> internal states and any other resources. It's true that it may not
> absolutely necessary for our GPUDriver. From my side, I'm ok to remove
> it.
>
> - `ExternalResourceDriver#retrieveResourceInfo` should not take
> `ResourceProfile` as argument. This exposes more information than it needs.
> In addition, it requires the runtime/core to understand how to properly
> wrap the external resource into `ResourceProfile`. E.g.,
> `ResourceProfile#extendedResources` takes `Resource`, which is an abstract
> class. Runtime/core has to known which implementation of `Resource` to use.
>
> True, at the moment, I think the amount of the resource is enough for
> the `ExternalResourceDriver#retrieveResourceInfo`. In the future, if
> the fine-grained external resource management is supported, the amount
> of the resource seems to be enough either. If we want to leverage some
> external resources which could not be measured by a single long value,
> we might enrich this. But I'd like to keep it out of the scope of this
> FLIP.
>
> - Do we really need `ExternalResourceInfo#getInformation`? I think it
> should be good enough to make `ExternalResourceInfo` an empty interface.
> User can define their own `ExternalResourceInfo` implementation and how it
> is used by the operator user codes.
>
> Sounds good.
>
> Best,
> Yangze Guo
>
> On Fri, Apr 10, 2020 at 6:04 PM Xintong Song <tonysong...@gmail.com> wrote:
> >
> > Sorry to pull this back. I have some concerns about the recent updated
> > interface details.
> >
> > - Should we have a factory interface for `ExternalResourceDriver`, that
> > takes the configuration and returns a driver instance? Otherwise, if we are
> > creating the driver instance with reflection, we kind of implicitly
> > requires the driver to have a public non-argument constructor. If we
> > decided to go with this approach, then we will not need
> > `ExternalResourceDriver#open`.
> > - Not sure about the necessity of `ExternalResourceDriver#close`. I would
> > suggest to avoid introduce more interfaces if not absolutely necessary.
> > - `ExternalResourceDriver#retrieveResourceInfo` should not take
> > `ResourceProfile` as argument. This exposes more information than it needs.
> > In addition, it requires the runtime/core to understand how to properly
> > wrap the external resource into `ResourceProfile`. E.g.,
> > `ResourceProfile#extendedResources` takes `Resource`, which is an abstract
> > class. Runtime/core has to known which implementation of `Resource` to use.
> > - Do we really need `ExternalResourceInfo#getInformation`? I think it
> > should be good enough to make `ExternalResourceInfo` an empty interface.
> > User can define their own `ExternalResourceInfo` implementation and how it
> > is used by the operator user codes.
> >
> > Thank you~
> >
> > Xintong Song
> >
> >
> >
> > On Thu, Apr 9, 2020 at 2:25 PM Becket Qin <becket....@gmail.com> wrote:
> >
> > > +1
> > >
> > > Thanks for driving this effort, Ynagze. The latest FLIP wiki looks good to
> > > me.
> > >
> > > Cheers,
> > >
> > > Jiangjie (Becket) Qin
> > >
> > > On Wed, Apr 8, 2020 at 4:10 PM Yangze Guo <karma...@gmail.com> wrote:
> > >
> > > > Edit: RuntimeContext interface
> > > > From: Map<String, Set<ExternalResourceInfo>>
> > > > getExternalResourceInfo(ResourceSpec resourceSpec);
> > > > To: Map<String, Set<ExternalResourceInfo>> getExternalResourceInfo();
> > > >
> > > > Best,
> > > > Yangze Guo
> > > >
> > > >
> > > > On Wed, Apr 8, 2020 at 11:36 AM Yangze Guo <karma...@gmail.com> wrote:
> > > > >
> > > > > Hi, there
> > > > >
> > > > > I have updated the FLIP, mainly target to make it more detailed and
> > > > > clear. The general design is not changed, but there are still some
> > > > > changes need to be notified here:
> > > > >
> > > > > - Change the `ExternalResourceDriver` from abstract class to
> > > > > interface, since it does not have an abstract implementation. Add life
> > > > > cycle method `open` and `close`.
> > > > >
> > > > > - Specify the method added to the RuntimeContext from which user get
> > > > > the information of external resources.
> > > > > Map<String, Set<ExternalResourceInfo>>
> > > > > getExternalResourceInfo(ResourceSpec resourceSpec);
> > > > >
> > > > > - Add "String getInformation()" method to `ExternalResourceInfo`
> > > > interface.
> > > > >
> > > > > - Treat adding external resource info to RestAPI/WebUI as a future
> > > work.
> > > > >
> > > > > If you have any new concerns after that change, please mentioned here.
> > > > > Sorry for disturbing you.
> > > > >
> > > > > Best,
> > > > > Yangze Guo
> > > > >
> > > > > On Wed, Apr 8, 2020 at 9:55 AM Yang Wang <danrtsey...@gmail.com>
> > > wrote:
> > > > > >
> > > > > > Thanks Yangze for the efforts to support GPU extended resources.
> > > > > >
> > > > > > +1 for this FLIP
> > > > > >
> > > > > >
> > > > > > Best,
> > > > > > Yang
> > > > > >
> > > > > > Till Rohrmann <trohrm...@apache.org> 于2020年4月2日周四 下午11:10写道：
> > > > > >
> > > > > > > Thanks for driving this effort Yangze.
> > > > > > >
> > > > > > > +1
> > > > > > >
> > > > > > > Cheers,
> > > > > > > Till
> > > > > > >
> > > > > > > On Wed, Apr 1, 2020 at 12:41 PM Canbin Zheng <
> > > felixzhen...@gmail.com
> > > > >
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Thanks Yangze for driving the initial CPU support!
> > > > > > > > +1 (non-binding) from my side.
> > > > > > > >
> > > > > > > >
> > > > > > > > Xintong Song <tonysong...@gmail.com> 于2020年4月1日周三 下午6:36写道：
> > > > > > > >
> > > > > > > > > Thanks Yangze, the FLIP looks good to me.
> > > > > > > > > +1 (non-binding) from my side.
> > > > > > > > >
> > > > > > > > > Thank you~
> > > > > > > > >
> > > > > > > > > Xintong Song
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Wed, Apr 1, 2020 at 5:22 PM Yangze Guo <karma...@gmail.com>
> > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hi everyone,
> > > > > > > > > >
> > > > > > > > > > I'd like to start the vote of FLIP-108 [1], which adds GPU
> > > > support in
> > > > > > > > > > Flink. This FLIP is discussed in the thread[2].
> > > > > > > > > >
> > > > > > > > > > The vote will be open for at least 72 hours. Unless there is
> > > an
> > > > > > > > > objection,
> > > > > > > > > > I will try to close it by April 4, 2020 10:00 UTC if we have
> > > > received
> > > > > > > > > > sufficient votes.
> > > > > > > > > >
> > > > > > > > > > [1]
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > >
> > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-108%3A+Add+GPU+support+in+Flink
> > > > > > > > > > [2]
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > >
> > > http://apache-flink-mailing-list-archive.1008284.n3.nabble.com/DISCUSS-FLIP-108-Add-GPU-support-in-Flink-td38286.html
> > > > > > > > > >
> > > > > > > > > > Best,
> > > > > > > > > > Yangze Guo
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > >
> > >
