Thanks for the explanation. I do not have a strong opinion regarding
this interface. So, if it is better from your perspective, I'm +1 for
this. I just saying it may not help a lot regarding the type-safe.
Regarding the bounded wildcard type, yes, it's the implementation
detail. If it won't make a difference for user, I'm also +1 for not
using bounded wildcard type there.
Best,
Yangze Guo
On Wed, Apr 15, 2020 at 4:23 PM Till Rohrmann <trohrm...@apache.org> wrote:
>
> I think <T extends ExternalResourceInfo> Set<T>
> getExternalResourceInfos(String resourceName, Class<T>
> externalResourceType) is not less flexible than the other API since you can
> always pass in ExternalResourceInfo.class as the second argument.
>
> The benefit I see for the user is that he does not have to do the
> instanceof checks and type casts himself. This is admittedly not a big deal
> but still a better API imo.
>
> I think the interface of the Driver and what is returned by the
> RuntimeContext don't have to have the same type because you can cast it or
> repack it. If the current implementation simply stores what the Driver
> returns and RuntimeContext returns this map, then it might seem that there
> is a connection. But this should be an implementation detail rather than a
> necessity.
>
> Maybe we could also pull in someone from the SDK team to give us his
> opinion on the user facing API.
>
> Cheers,
> Till
>
> On Wed, Apr 15, 2020 at 10:13 AM Xintong Song <tonysong...@gmail.com> wrote:
>
> > >
> > > I agree that such an interface won't give compile time checks but I think
> > > that it could be easier to use from a user's perspective because there is
> > > no explicit casting required.
> > > public interface RuntimeContext {
> > > <T extends ExternalResourceInfo> Set<T>
> > getExternalResourceInfos(String
> > > resourceName, Class<T> externalResourceType);
> > > }
> >
> >
> > I'm not sure how less efforts is required from users to pass in a
> > `externalResourceType` compared to do an explicit type casting.
> > A potential side effect of passing in a `externalResourceType` is that, it
> > requires user (e.g. the operator) to know which specific type should be
> > returned in advance, which may limit the flexibility.
> >
> > E.g., we might have an operator that can work with multiple different
> > implementations of `ExternalResourceInfo`. It may decide its behavior based
> > on the actually type returned by `getExternalResourceInfos` at runtime.
> >
> >
> > Thank you~
> >
> > Xintong Song
> >
> >
> >
> > On Wed, Apr 15, 2020 at 4:09 PM Yangze Guo <karma...@gmail.com> wrote:
> >
> > > @Till
> > > If we add "Class<T> externalResourceType" param, what if there are
> > > multiple subtypes in the ExternalResourceInfos set of one external
> > > resource? It seems user has to set the T to ExternalResourceInfo and
> > > the mechanism is useless at this case.
> > >
> > > Best,
> > > Yangze Guo
> > >
> > > On Wed, Apr 15, 2020 at 3:57 PM Till Rohrmann <trohrm...@apache.org>
> > > wrote:
> > > >
> > > > Ok, if there can be multiple resources of the same type then we
> > > definitely
> > > > need the name as a differentiator.
> > > >
> > > > I agree that such an interface won't give compile time checks but I
> > think
> > > > that it could be easier to use from a user's perspective because there
> > is
> > > > no explicit casting required.
> > > >
> > > > public interface RuntimeContext {
> > > > <T extends ExternalResourceInfo> Set<T>
> > > getExternalResourceInfos(String
> > > > resourceName, Class<T> externalResourceType);
> > > > }
> > > >
> > > > One minor note: I think the value of the returned map does not need to
> > > use
> > > > a bounded wildcard type because for the user it won't make a
> > difference.
> > > >
> > > > Cheers,
> > > > Till
> > > >
> > > > On Wed, Apr 15, 2020 at 8:20 AM Yangze Guo <karma...@gmail.com> wrote:
> > > >
> > > > > Hi Till,
> > > > >
> > > > > > ExternalResourceDriver could return a Set<? extends
> > > > > ExternalResourceInfo>.
> > > > > It sounds good.
> > > > >
> > > > > > then one could make the interface type-safe by changing it to
> > > > > > public interface RuntimeContext {
> > > > > > <T extends ExternalResourceInfo> Set<T>
> > > > > > getExternalResourceInfo(Class<T> externalResourceType);
> > > > > > }
> > > > > I think it may not help.
> > > > > - I think the assumption of "there is always only one resource of a
> > > > > specific type" is too strong. The external resource framework should
> > > > > only assume it gets a set of ExternalResourceInfo from the driver.
> > The
> > > > > concrete implementation is given by user. So, if we give such an
> > > > > assumption, it would hurt the flexibility. There could be multiple
> > > > > types in the returned externalResourceInfo set. There could also be
> > > > > different types returned from different driver implementation or
> > > > > version. The contract about the return type between Driver and
> > > > > Operator should be guaranteed by user.
> > > > > - Since the Drivers are loaded dynamically in runtime, if there is a
> > > > > type mismatch, the job would fail in runtime instead of in compile
> > > > > time, no matter the type extraction is done by Operator or Flink
> > core.
> > > > > This interface would not gain benefits for type safety.
> > > > >
> > > > > Best,
> > > > > Yangze Guo
> > > > >
> > > > > On Wed, Apr 15, 2020 at 1:38 AM Till Rohrmann <trohrm...@apache.org>
> > > > > wrote:
> > > > > >
> > > > > > 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
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > >
> > >
> >
