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
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > >
> >
