Thanks for updating the FLIP, Yangze.
If ExternalResourceInfo is a marker interface, then ExternalResourceDriver
could return a Set<? extends ExternalResourceInfo>. This makes is a bit
nicer for the implementor because he can use the concrete subtype.
If we assume that users will always cast the ExternalResourceInfo instance
into the concrete subtype and if we assume that there is always only one
resource of a specific type, then one could make the interface type-safe by
changing it to
public interface RuntimeContext {
<T extends ExternalResourceInfo> Set<T>
getExternalResourceInfo(Class<T> externalResourceType);
}
If we want to support multiple GPU resources, then one would need to use
the name of the respective resource as well.
Cheers,
Till
On Mon, Apr 13, 2020 at 4:19 AM Xintong Song <tonysong...@gmail.com> wrote:
> Thanks for updating the FLIP, Yangze.
> The latest FLIP looks good to me.
>
> nit: Javadoc of `ExternalResourceDriver#retrieveResourceInfo` is out of
> sync.
>
> > Retrieve the information of the external resources according to the
> > resourceProfile.
>
>
> Thank you~
>
> Xintong Song
>
>
>
> On Sat, Apr 11, 2020 at 11:04 AM Becket Qin <becket....@gmail.com> wrote:
>
> > Good feedback form Xintong. The latest FLIP looks good to me.
> >
> > Thanks,
> >
> > Jiangjie (Becket) Qin
> >
> > On Sat, Apr 11, 2020 at 9:20 AM Yangze Guo <karma...@gmail.com> wrote:
> >
> > > 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
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > >
> > > > > >
> > >
> >
>
