(DEPRECATED) Apache Flink Mailing List archive.

[PROPOSAL] Merge NewClusterClient into ClusterClient

Classic

List

Threaded

8 messages Options

tison

[PROPOSAL] Merge NewClusterClient into ClusterClient

Hi devs,

FLINK-14096[1] was created yesterday. It is aimed at merge the bridge
class NewClusterClient into ClusterClient because with the effort
under FLINK-10392 this bridge class is no longer necessary.

Technically in current codebase all implementation of interface
NewClusterClient is subclass of ClusterClient so that the work
required is no more than move method declaration. It helps we use
type signature ClusterClient instead of
<ClusterClient & NewClusterClient or even cannot represent the
latter if we aren't in a type variable context. This should not affect
anything internal in Flink scope.

However, as mentioned by Kostas in the JIRA and a previous discussion
under a commit[2], it seems that we provide some levels of backward
compatibility for ClusterClient and thus it's better to start a public
discussion here.

There are two concerns from my side.

1. How much impact this proposal brings to users programming directly
to ClusterClient?

The specific changes here are add two methods `submitJob` and
`requestJobResult` which are already implemented by RestClusterClient
and MiniClusterClient. Users would only be affected if they create
a class that inherits ClusterClient and doesn't implement these
methods. Besides, users who create a class that implements
NewClusterClient would be affected by the removal of NewClusterClient.

If we have to provide backward compatibility and the impact is no
further than those above, we can deprecate NewClusterClient, merge
the methods into ClusterClient with a dummy default like throw
Exception.

2. Why do we provide backward compatibility for ClusterClient?

It already surprises Kostas and me while we think ClusterClient is a
totally internal class which we can evolve regardless of api
stability. Our community promises api stability by marking class
and/or method as @Public/@PublicEvolving. It is wried and even
dangerous we are somehow enforced to provide backward compatibility
for classes without any annotation.

Besides, as I mention in [2], users who anyway want to program
directly to internal classes/interfaces are considered to prepare to
make adaptations when bump version of Flink.

Best,
tison.

[1] https://issues.apache.org/jira/browse/FLINK-14096
[2]
https://github.com/apache/flink/commit/dc9e4494dddfed36432e6bbf6cd3231530bc2e01

Jeff Zhang

Re: [PROPOSAL] Merge NewClusterClient into ClusterClient

Thanks for raising this discussion. Overall +1 to merge NewClusterClient
into ClusterClient.

1. I think it is OK to break the backward compatibility. This current
client api is no so clean which already cause issue for downstream project
and flink itself.
In flink scala shell, I notice this kind of non-readable code Option[Either
[MiniCluster , ClusterClient[_]]])
https://github.com/apache/flink/blob/master/flink-scala-shell/src/main/scala/org/apache/flink/api/scala/FlinkShell.scala#L138
I also created tickets and PR to try to simply it.
https://github.com/apache/flink/pull/8546
https://github.com/apache/flink/pull/8533
Personally I don't think we need to keep backward compatibility for
non-well-designed api, otherwise it will bring lots of unnecessary
overhead.

2. Another concern is that I notice there're many implementation details in
ClusterClient. I think we should just expose a thin interface, so maybe we
can create interface ClusterClient which includes as less methods as
possible, and move all the implementation to AbstractClusterClient.

Zili Chen <[hidden email]> 于2019年9月18日周三上午9:46写道：

> Hi devs,
>
> FLINK-14096[1] was created yesterday. It is aimed at merge the bridge
> class NewClusterClient into ClusterClient because with the effort
> under FLINK-10392 this bridge class is no longer necessary.
>
> Technically in current codebase all implementation of interface
> NewClusterClient is subclass of ClusterClient so that the work
> required is no more than move method declaration. It helps we use
> type signature ClusterClient instead of
> <ClusterClient & NewClusterClient or even cannot represent the
> latter if we aren't in a type variable context. This should not affect
> anything internal in Flink scope.
>
> However, as mentioned by Kostas in the JIRA and a previous discussion
> under a commit[2], it seems that we provide some levels of backward
> compatibility for ClusterClient and thus it's better to start a public
> discussion here.
>
> There are two concerns from my side.
>
> 1. How much impact this proposal brings to users programming directly
> to ClusterClient?
>
> The specific changes here are add two methods `submitJob` and
> `requestJobResult` which are already implemented by RestClusterClient
> and MiniClusterClient. Users would only be affected if they create
> a class that inherits ClusterClient and doesn't implement these
> methods. Besides, users who create a class that implements
> NewClusterClient would be affected by the removal of NewClusterClient.
>
> If we have to provide backward compatibility and the impact is no
> further than those above, we can deprecate NewClusterClient, merge
> the methods into ClusterClient with a dummy default like throw
> Exception.
>
> 2. Why do we provide backward compatibility for ClusterClient?
>
> It already surprises Kostas and me while we think ClusterClient is a
> totally internal class which we can evolve regardless of api
> stability. Our community promises api stability by marking class
> and/or method as @Public/@PublicEvolving. It is wried and even
> dangerous we are somehow enforced to provide backward compatibility
> for classes without any annotation.
>
> Besides, as I mention in [2], users who anyway want to program
> directly to internal classes/interfaces are considered to prepare to
> make adaptations when bump version of Flink.
>
> Best,
> tison.
>
> [1] https://issues.apache.org/jira/browse/FLINK-14096
> [2]
>
> https://github.com/apache/flink/commit/dc9e4494dddfed36432e6bbf6cd3231530bc2e01
>

--
Best Regards

Jeff Zhang

tison

Re: [PROPOSAL] Merge NewClusterClient into ClusterClient

Hi Jeff,

Thanks for your reply.

The ongoing client API enhancement thread[1] is mainly aimed at dealing with
issues of our client API, as you mentioned, current client API is no so
clean.

Because client API naturally becomes public & user-facing inteface it is
expected that we start a series of discussions for how the inteface should
look like. However, it isn't expected that we have to talk about backward
compatibility too much in this scope.

I agree that it is painful if we always keep compatibility for
non-well-designed API. Even in this specific scenario we bring such to
Public.
It is mentioned in the discussion under [2] that I think it could be the
time
or so to discuss our InterfaceAudience policy. At least it would be a pity
if
we don't address this InterfaceAudience issue towards 2.0. But let's say it
could be a separated thread.

For expose a thin interface and move all the implementation to
AbstractClusterClient, I think the community consensus is towards an
ClusterClient interface and thus there is no need for an
AbstractClusterClient.
For implementation details, it is mainly about a series of #run methods. We
will gradually exclude them from ClusterClient and it is the responsibility
of Executor in the design document[3] to take care of compilation and
deployment.

BTW, I take a look at the pull requests you link to. In fact I create a
similar issue[4] and also consider simplify code in flink-scala-shell. Let's
move the detailed discussion to the corresponding issues and pull requests
or start another thread then. I don't intend to cover a lot of concerns
generally under this thread :-)

Best,
tison.

[1]
https://lists.apache.org/thread.html/ce99cba4a10b9dc40eb729d39910f315ae41d80ec74f09a356c73938@%3Cdev.flink.apache.org%3E
[2]
https://github.com/apache/flink/commit/dc9e4494dddfed36432e6bbf6cd3231530bc2e01#commitcomment-34980406
[3]
https://docs.google.com/document/d/1E-8UjOLz4QPUTxetGWbU23OlsIH9VIdodpTsxwoQTs0/edit#
[4] https://issues.apache.org/jira/browse/FLINK-13961

Jeff Zhang <[hidden email]> 于2019年9月18日周三上午10:49写道：

> Thanks for raising this discussion. Overall +1 to merge NewClusterClient
> into ClusterClient.
>
> 1. I think it is OK to break the backward compatibility. This current
> client api is no so clean which already cause issue for downstream project
> and flink itself.
> In flink scala shell, I notice this kind of non-readable code
> Option[Either
> [MiniCluster , ClusterClient[_]]])
>
> https://github.com/apache/flink/blob/master/flink-scala-shell/src/main/scala/org/apache/flink/api/scala/FlinkShell.scala#L138
> I also created tickets and PR to try to simply it.
> https://github.com/apache/flink/pull/8546
> https://github.com/apache/flink/pull/8533
> Personally I don't think we need to keep backward compatibility for
> non-well-designed api, otherwise it will bring lots of unnecessary
> overhead.
>
> 2. Another concern is that I notice there're many implementation details in
> ClusterClient. I think we should just expose a thin interface, so maybe we
> can create interface ClusterClient which includes as less methods as
> possible, and move all the implementation to AbstractClusterClient.
>
>
>
>
>
>
>
> Zili Chen <[hidden email]> 于2019年9月18日周三上午9:46写道：
>
> > Hi devs,
> >
> > FLINK-14096[1] was created yesterday. It is aimed at merge the bridge
> > class NewClusterClient into ClusterClient because with the effort
> > under FLINK-10392 this bridge class is no longer necessary.
> >
> > Technically in current codebase all implementation of interface
> > NewClusterClient is subclass of ClusterClient so that the work
> > required is no more than move method declaration. It helps we use
> > type signature ClusterClient instead of
> > <ClusterClient & NewClusterClient or even cannot represent the
> > latter if we aren't in a type variable context. This should not affect
> > anything internal in Flink scope.
> >
> > However, as mentioned by Kostas in the JIRA and a previous discussion
> > under a commit[2], it seems that we provide some levels of backward
> > compatibility for ClusterClient and thus it's better to start a public
> > discussion here.
> >
> > There are two concerns from my side.
> >
> > 1. How much impact this proposal brings to users programming directly
> > to ClusterClient?
> >
> > The specific changes here are add two methods `submitJob` and
> > `requestJobResult` which are already implemented by RestClusterClient
> > and MiniClusterClient. Users would only be affected if they create
> > a class that inherits ClusterClient and doesn't implement these
> > methods. Besides, users who create a class that implements
> > NewClusterClient would be affected by the removal of NewClusterClient.
> >
> > If we have to provide backward compatibility and the impact is no
> > further than those above, we can deprecate NewClusterClient, merge
> > the methods into ClusterClient with a dummy default like throw
> > Exception.
> >
> > 2. Why do we provide backward compatibility for ClusterClient?
> >
> > It already surprises Kostas and me while we think ClusterClient is a
> > totally internal class which we can evolve regardless of api
> > stability. Our community promises api stability by marking class
> > and/or method as @Public/@PublicEvolving. It is wried and even
> > dangerous we are somehow enforced to provide backward compatibility
> > for classes without any annotation.
> >
> > Besides, as I mention in [2], users who anyway want to program
> > directly to internal classes/interfaces are considered to prepare to
> > make adaptations when bump version of Flink.
> >
> > Best,
> > tison.
> >
> > [1] https://issues.apache.org/jira/browse/FLINK-14096
> > [2]
> >
> >
> https://github.com/apache/flink/commit/dc9e4494dddfed36432e6bbf6cd3231530bc2e01
> >
>
>
> --
> Best Regards
>
> Jeff Zhang
>

SHI Xiaogang

Re: [PROPOSAL] Merge NewClusterClient into ClusterClient

In reply to this post by Jeff Zhang

Hi Tison,

Thanks for bringing this.

I think it's fine to break the back compatibility of client API now that
ClusterClient is not well designed for public usage.
But from my perspective, we should postpone any modification to existing
interfaces until we come to an agreement on new client API. Otherwise, our
users may adapt their implementation more than once.

Regards,
Xiaogang

Jeff Zhang <[hidden email]> 于2019年9月18日周三上午10:49写道：

tison

Re: [PROPOSAL] Merge NewClusterClient into ClusterClient

Hi Xiaogang,

Thanks for your reply.

According to the feature discussion thread[1] client API enhancement is a
planned
feature towards 1.10 and thus I think this thread is valid if we can reach
a consensus
and introduce new client API in this development cycle.

Best,
tison.

[1]
https://lists.apache.org/thread.html/22639ca7de62a18f50e90db53e73910bd99b7f00c82f7494f4cb035f@%3Cdev.flink.apache.org%3E

SHI Xiaogang <[hidden email]> 于2019年9月18日周三下午3:03写道：

> Hi Tison,
>
> Thanks for bringing this.
>
> I think it's fine to break the back compatibility of client API now that
> ClusterClient is not well designed for public usage.
> But from my perspective, we should postpone any modification to existing
> interfaces until we come to an agreement on new client API. Otherwise, our
> users may adapt their implementation more than once.
>
> Regards,
> Xiaogang
>
> Jeff Zhang <[hidden email]> 于2019年9月18日周三上午10:49写道：
>
> > Thanks for raising this discussion. Overall +1 to merge NewClusterClient
> > into ClusterClient.
> >
> > 1. I think it is OK to break the backward compatibility. This current
> > client api is no so clean which already cause issue for downstream
> project
> > and flink itself.
> > In flink scala shell, I notice this kind of non-readable code
> > Option[Either
> > [MiniCluster , ClusterClient[_]]])
> >
> >
> https://github.com/apache/flink/blob/master/flink-scala-shell/src/main/scala/org/apache/flink/api/scala/FlinkShell.scala#L138
> > I also created tickets and PR to try to simply it.
> > https://github.com/apache/flink/pull/8546
> > https://github.com/apache/flink/pull/8533
> > Personally I don't think we need to keep backward compatibility for
> > non-well-designed api, otherwise it will bring lots of unnecessary
> > overhead.
> >
> > 2. Another concern is that I notice there're many implementation details
> in
> > ClusterClient. I think we should just expose a thin interface, so maybe
> we
> > can create interface ClusterClient which includes as less methods as
> > possible, and move all the implementation to AbstractClusterClient.
> >
> >
> >
> >
> >
> >
> >
> > Zili Chen <[hidden email]> 于2019年9月18日周三上午9:46写道：
> >
> > > Hi devs,
> > >
> > > FLINK-14096[1] was created yesterday. It is aimed at merge the bridge
> > > class NewClusterClient into ClusterClient because with the effort
> > > under FLINK-10392 this bridge class is no longer necessary.
> > >
> > > Technically in current codebase all implementation of interface
> > > NewClusterClient is subclass of ClusterClient so that the work
> > > required is no more than move method declaration. It helps we use
> > > type signature ClusterClient instead of
> > > <ClusterClient & NewClusterClient or even cannot represent the
> > > latter if we aren't in a type variable context. This should not affect
> > > anything internal in Flink scope.
> > >
> > > However, as mentioned by Kostas in the JIRA and a previous discussion
> > > under a commit[2], it seems that we provide some levels of backward
> > > compatibility for ClusterClient and thus it's better to start a public
> > > discussion here.
> > >
> > > There are two concerns from my side.
> > >
> > > 1. How much impact this proposal brings to users programming directly
> > > to ClusterClient?
> > >
> > > The specific changes here are add two methods `submitJob` and
> > > `requestJobResult` which are already implemented by RestClusterClient
> > > and MiniClusterClient. Users would only be affected if they create
> > > a class that inherits ClusterClient and doesn't implement these
> > > methods. Besides, users who create a class that implements
> > > NewClusterClient would be affected by the removal of NewClusterClient.
> > >
> > > If we have to provide backward compatibility and the impact is no
> > > further than those above, we can deprecate NewClusterClient, merge
> > > the methods into ClusterClient with a dummy default like throw
> > > Exception.
> > >
> > > 2. Why do we provide backward compatibility for ClusterClient?
> > >
> > > It already surprises Kostas and me while we think ClusterClient is a
> > > totally internal class which we can evolve regardless of api
> > > stability. Our community promises api stability by marking class
> > > and/or method as @Public/@PublicEvolving. It is wried and even
> > > dangerous we are somehow enforced to provide backward compatibility
> > > for classes without any annotation.
> > >
> > > Besides, as I mention in [2], users who anyway want to program
> > > directly to internal classes/interfaces are considered to prepare to
> > > make adaptations when bump version of Flink.
> > >
> > > Best,
> > > tison.
> > >
> > > [1] https://issues.apache.org/jira/browse/FLINK-14096
> > > [2]
> > >
> > >
> >
> https://github.com/apache/flink/commit/dc9e4494dddfed36432e6bbf6cd3231530bc2e01
> > >
> >
> >
> > --
> > Best Regards
> >
> > Jeff Zhang
> >
>

Aljoscha Krettek-2

Re: [PROPOSAL] Merge NewClusterClient into ClusterClient

I agree that NewClusterClient and ClusterClient can be merged now that there is no pre-FLIP-6 code base anymore.

Side note, there are a lot of methods in ClusterClient that should not really be there, in my opinion:
- all the getOptimizedPlan*() method
- the run() methods. In the end, only submitJob should be required

We should also see what Till (cc’ed) says, maybe he has an opinion on why the separation should be kept.

Best,
Aljoscha

> On 18. Sep 2019, at 11:54, Zili Chen <[hidden email]> wrote:
>
> Hi Xiaogang,
>
> Thanks for your reply.
>
> According to the feature discussion thread[1] client API enhancement is a
> planned
> feature towards 1.10 and thus I think this thread is valid if we can reach
> a consensus
> and introduce new client API in this development cycle.
>
> Best,
> tison.
>
> [1]
> https://lists.apache.org/thread.html/22639ca7de62a18f50e90db53e73910bd99b7f00c82f7494f4cb035f@%3Cdev.flink.apache.org%3E
>
>
> SHI Xiaogang <[hidden email]> 于2019年9月18日周三下午3:03写道：
>
>> Hi Tison,
>>
>> Thanks for bringing this.
>>
>> I think it's fine to break the back compatibility of client API now that
>> ClusterClient is not well designed for public usage.
>> But from my perspective, we should postpone any modification to existing
>> interfaces until we come to an agreement on new client API. Otherwise, our
>> users may adapt their implementation more than once.
>>
>> Regards,
>> Xiaogang
>>
>> Jeff Zhang <[hidden email]> 于2019年9月18日周三上午10:49写道：
>>
>>> Thanks for raising this discussion. Overall +1 to merge NewClusterClient
>>> into ClusterClient.
>>>
>>> 1. I think it is OK to break the backward compatibility. This current
>>> client api is no so clean which already cause issue for downstream
>> project
>>> and flink itself.
>>> In flink scala shell, I notice this kind of non-readable code
>>> Option[Either
>>> [MiniCluster , ClusterClient[_]]])
>>>
>>>
>> https://github.com/apache/flink/blob/master/flink-scala-shell/src/main/scala/org/apache/flink/api/scala/FlinkShell.scala#L138
>>> I also created tickets and PR to try to simply it.
>>> https://github.com/apache/flink/pull/8546
>>> https://github.com/apache/flink/pull/8533
>>> Personally I don't think we need to keep backward compatibility for
>>> non-well-designed api, otherwise it will bring lots of unnecessary
>>> overhead.
>>>
>>> 2. Another concern is that I notice there're many implementation details
>> in
>>> ClusterClient. I think we should just expose a thin interface, so maybe
>> we
>>> can create interface ClusterClient which includes as less methods as
>>> possible, and move all the implementation to AbstractClusterClient.
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> Zili Chen <[hidden email]> 于2019年9月18日周三上午9:46写道：
>>>
>>>> Hi devs,
>>>>
>>>> FLINK-14096[1] was created yesterday. It is aimed at merge the bridge
>>>> class NewClusterClient into ClusterClient because with the effort
>>>> under FLINK-10392 this bridge class is no longer necessary.
>>>>
>>>> Technically in current codebase all implementation of interface
>>>> NewClusterClient is subclass of ClusterClient so that the work
>>>> required is no more than move method declaration. It helps we use
>>>> type signature ClusterClient instead of
>>>> <ClusterClient & NewClusterClient or even cannot represent the
>>>> latter if we aren't in a type variable context. This should not affect
>>>> anything internal in Flink scope.
>>>>
>>>> However, as mentioned by Kostas in the JIRA and a previous discussion
>>>> under a commit[2], it seems that we provide some levels of backward
>>>> compatibility for ClusterClient and thus it's better to start a public
>>>> discussion here.
>>>>
>>>> There are two concerns from my side.
>>>>
>>>> 1. How much impact this proposal brings to users programming directly
>>>> to ClusterClient?
>>>>
>>>> The specific changes here are add two methods `submitJob` and
>>>> `requestJobResult` which are already implemented by RestClusterClient
>>>> and MiniClusterClient. Users would only be affected if they create
>>>> a class that inherits ClusterClient and doesn't implement these
>>>> methods. Besides, users who create a class that implements
>>>> NewClusterClient would be affected by the removal of NewClusterClient.
>>>>
>>>> If we have to provide backward compatibility and the impact is no
>>>> further than those above, we can deprecate NewClusterClient, merge
>>>> the methods into ClusterClient with a dummy default like throw
>>>> Exception.
>>>>
>>>> 2. Why do we provide backward compatibility for ClusterClient?
>>>>
>>>> It already surprises Kostas and me while we think ClusterClient is a
>>>> totally internal class which we can evolve regardless of api
>>>> stability. Our community promises api stability by marking class
>>>> and/or method as @Public/@PublicEvolving. It is wried and even
>>>> dangerous we are somehow enforced to provide backward compatibility
>>>> for classes without any annotation.
>>>>
>>>> Besides, as I mention in [2], users who anyway want to program
>>>> directly to internal classes/interfaces are considered to prepare to
>>>> make adaptations when bump version of Flink.
>>>>
>>>> Best,
>>>> tison.
>>>>
>>>> [1] https://issues.apache.org/jira/browse/FLINK-14096
>>>> [2]
>>>>
>>>>
>>>
>> https://github.com/apache/flink/commit/dc9e4494dddfed36432e6bbf6cd3231530bc2e01
>>>>
>>>
>>>
>>> --
>>> Best Regards
>>>
>>> Jeff Zhang
>>>
>>

Till Rohrmann

Re: [PROPOSAL] Merge NewClusterClient into ClusterClient

No reason to keep the separation. The NewClusterClient interface was only
introduced to add new methods and not having to implement them for the
other ClusterClient implementations.

Cheers,
Till

On Wed, Sep 18, 2019 at 3:17 PM Aljoscha Krettek <[hidden email]>
wrote:

> I agree that NewClusterClient and ClusterClient can be merged now that
> there is no pre-FLIP-6 code base anymore.
>
> Side note, there are a lot of methods in ClusterClient that should not
> really be there, in my opinion:
> - all the getOptimizedPlan*() method
> - the run() methods. In the end, only submitJob should be required
>
> We should also see what Till (cc’ed) says, maybe he has an opinion on why
> the separation should be kept.
>
> Best,
> Aljoscha
>
> > On 18. Sep 2019, at 11:54, Zili Chen <[hidden email]> wrote:
> >
> > Hi Xiaogang,
> >
> > Thanks for your reply.
> >
> > According to the feature discussion thread[1] client API enhancement is a
> > planned
> > feature towards 1.10 and thus I think this thread is valid if we can
> reach
> > a consensus
> > and introduce new client API in this development cycle.
> >
> > Best,
> > tison.
> >
> > [1]
> >
> https://lists.apache.org/thread.html/22639ca7de62a18f50e90db53e73910bd99b7f00c82f7494f4cb035f@%3Cdev.flink.apache.org%3E
> >
> >
> > SHI Xiaogang <[hidden email]> 于2019年9月18日周三下午3:03写道：
> >
> >> Hi Tison,
> >>
> >> Thanks for bringing this.
> >>
> >> I think it's fine to break the back compatibility of client API now that
> >> ClusterClient is not well designed for public usage.
> >> But from my perspective, we should postpone any modification to existing
> >> interfaces until we come to an agreement on new client API. Otherwise,
> our
> >> users may adapt their implementation more than once.
> >>
> >> Regards,
> >> Xiaogang
> >>
> >> Jeff Zhang <[hidden email]> 于2019年9月18日周三上午10:49写道：
> >>
> >>> Thanks for raising this discussion. Overall +1 to merge
> NewClusterClient
> >>> into ClusterClient.
> >>>
> >>> 1. I think it is OK to break the backward compatibility. This current
> >>> client api is no so clean which already cause issue for downstream
> >> project
> >>> and flink itself.
> >>> In flink scala shell, I notice this kind of non-readable code
> >>> Option[Either
> >>> [MiniCluster , ClusterClient[_]]])
> >>>
> >>>
> >>
> https://github.com/apache/flink/blob/master/flink-scala-shell/src/main/scala/org/apache/flink/api/scala/FlinkShell.scala#L138
> >>> I also created tickets and PR to try to simply it.
> >>> https://github.com/apache/flink/pull/8546
> >>> https://github.com/apache/flink/pull/8533
> >>> Personally I don't think we need to keep backward compatibility for
> >>> non-well-designed api, otherwise it will bring lots of unnecessary
> >>> overhead.
> >>>
> >>> 2. Another concern is that I notice there're many implementation
> details
> >> in
> >>> ClusterClient. I think we should just expose a thin interface, so maybe
> >> we
> >>> can create interface ClusterClient which includes as less methods as
> >>> possible, and move all the implementation to AbstractClusterClient.
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>> Zili Chen <[hidden email]> 于2019年9月18日周三上午9:46写道：
> >>>
> >>>> Hi devs,
> >>>>
> >>>> FLINK-14096[1] was created yesterday. It is aimed at merge the bridge
> >>>> class NewClusterClient into ClusterClient because with the effort
> >>>> under FLINK-10392 this bridge class is no longer necessary.
> >>>>
> >>>> Technically in current codebase all implementation of interface
> >>>> NewClusterClient is subclass of ClusterClient so that the work
> >>>> required is no more than move method declaration. It helps we use
> >>>> type signature ClusterClient instead of
> >>>> <ClusterClient & NewClusterClient or even cannot represent the
> >>>> latter if we aren't in a type variable context. This should not affect
> >>>> anything internal in Flink scope.
> >>>>
> >>>> However, as mentioned by Kostas in the JIRA and a previous discussion
> >>>> under a commit[2], it seems that we provide some levels of backward
> >>>> compatibility for ClusterClient and thus it's better to start a public
> >>>> discussion here.
> >>>>
> >>>> There are two concerns from my side.
> >>>>
> >>>> 1. How much impact this proposal brings to users programming directly
> >>>> to ClusterClient?
> >>>>
> >>>> The specific changes here are add two methods `submitJob` and
> >>>> `requestJobResult` which are already implemented by RestClusterClient
> >>>> and MiniClusterClient. Users would only be affected if they create
> >>>> a class that inherits ClusterClient and doesn't implement these
> >>>> methods. Besides, users who create a class that implements
> >>>> NewClusterClient would be affected by the removal of NewClusterClient.
> >>>>
> >>>> If we have to provide backward compatibility and the impact is no
> >>>> further than those above, we can deprecate NewClusterClient, merge
> >>>> the methods into ClusterClient with a dummy default like throw
> >>>> Exception.
> >>>>
> >>>> 2. Why do we provide backward compatibility for ClusterClient?
> >>>>
> >>>> It already surprises Kostas and me while we think ClusterClient is a
> >>>> totally internal class which we can evolve regardless of api
> >>>> stability. Our community promises api stability by marking class
> >>>> and/or method as @Public/@PublicEvolving. It is wried and even
> >>>> dangerous we are somehow enforced to provide backward compatibility
> >>>> for classes without any annotation.
> >>>>
> >>>> Besides, as I mention in [2], users who anyway want to program
> >>>> directly to internal classes/interfaces are considered to prepare to
> >>>> make adaptations when bump version of Flink.
> >>>>
> >>>> Best,
> >>>> tison.
> >>>>
> >>>> [1] https://issues.apache.org/jira/browse/FLINK-14096
> >>>> [2]
> >>>>
> >>>>
> >>>
> >>
> https://github.com/apache/flink/commit/dc9e4494dddfed36432e6bbf6cd3231530bc2e01
> >>>>
> >>>
> >>>
> >>> --
> >>> Best Regards
> >>>
> >>> Jeff Zhang
> >>>
> >>
>
>

tison

Re: [PROPOSAL] Merge NewClusterClient into ClusterClient

Thanks for your suggestions and information.

I'll therefore reopen the corresponding pull request.

Best,
tison.

Till Rohrmann <[hidden email]> 于2019年9月18日周三下午10:55写道：

> No reason to keep the separation. The NewClusterClient interface was only
> introduced to add new methods and not having to implement them for the
> other ClusterClient implementations.
>
> Cheers,
> Till
>
> On Wed, Sep 18, 2019 at 3:17 PM Aljoscha Krettek <[hidden email]>
> wrote:
>
> > I agree that NewClusterClient and ClusterClient can be merged now that
> > there is no pre-FLIP-6 code base anymore.
> >
> > Side note, there are a lot of methods in ClusterClient that should not
> > really be there, in my opinion:
> > - all the getOptimizedPlan*() method
> > - the run() methods. In the end, only submitJob should be required
> >
> > We should also see what Till (cc’ed) says, maybe he has an opinion on why
> > the separation should be kept.
> >
> > Best,
> > Aljoscha
> >
> > > On 18. Sep 2019, at 11:54, Zili Chen <[hidden email]> wrote:
> > >
> > > Hi Xiaogang,
> > >
> > > Thanks for your reply.
> > >
> > > According to the feature discussion thread[1] client API enhancement
> is a
> > > planned
> > > feature towards 1.10 and thus I think this thread is valid if we can
> > reach
> > > a consensus
> > > and introduce new client API in this development cycle.
> > >
> > > Best,
> > > tison.
> > >
> > > [1]
> > >
> >
> https://lists.apache.org/thread.html/22639ca7de62a18f50e90db53e73910bd99b7f00c82f7494f4cb035f@%3Cdev.flink.apache.org%3E
> > >
> > >
> > > SHI Xiaogang <[hidden email]> 于2019年9月18日周三下午3:03写道：
> > >
> > >> Hi Tison,
> > >>
> > >> Thanks for bringing this.
> > >>
> > >> I think it's fine to break the back compatibility of client API now
> that
> > >> ClusterClient is not well designed for public usage.
> > >> But from my perspective, we should postpone any modification to
> existing
> > >> interfaces until we come to an agreement on new client API. Otherwise,
> > our
> > >> users may adapt their implementation more than once.
> > >>
> > >> Regards,
> > >> Xiaogang
> > >>
> > >> Jeff Zhang <[hidden email]> 于2019年9月18日周三上午10:49写道：
> > >>
> > >>> Thanks for raising this discussion. Overall +1 to merge
> > NewClusterClient
> > >>> into ClusterClient.
> > >>>
> > >>> 1. I think it is OK to break the backward compatibility. This current
> > >>> client api is no so clean which already cause issue for downstream
> > >> project
> > >>> and flink itself.
> > >>> In flink scala shell, I notice this kind of non-readable code
> > >>> Option[Either
> > >>> [MiniCluster , ClusterClient[_]]])
> > >>>
> > >>>
> > >>
> >
> https://github.com/apache/flink/blob/master/flink-scala-shell/src/main/scala/org/apache/flink/api/scala/FlinkShell.scala#L138
> > >>> I also created tickets and PR to try to simply it.
> > >>> https://github.com/apache/flink/pull/8546
> > >>> https://github.com/apache/flink/pull/8533
> > >>> Personally I don't think we need to keep backward compatibility for
> > >>> non-well-designed api, otherwise it will bring lots of unnecessary
> > >>> overhead.
> > >>>
> > >>> 2. Another concern is that I notice there're many implementation
> > details
> > >> in
> > >>> ClusterClient. I think we should just expose a thin interface, so
> maybe
> > >> we
> > >>> can create interface ClusterClient which includes as less methods as
> > >>> possible, and move all the implementation to AbstractClusterClient.
> > >>>
> > >>>
> > >>>
> > >>>
> > >>>
> > >>>
> > >>>
> > >>> Zili Chen <[hidden email]> 于2019年9月18日周三上午9:46写道：
> > >>>
> > >>>> Hi devs,
> > >>>>
> > >>>> FLINK-14096[1] was created yesterday. It is aimed at merge the
> bridge
> > >>>> class NewClusterClient into ClusterClient because with the effort
> > >>>> under FLINK-10392 this bridge class is no longer necessary.
> > >>>>
> > >>>> Technically in current codebase all implementation of interface
> > >>>> NewClusterClient is subclass of ClusterClient so that the work
> > >>>> required is no more than move method declaration. It helps we use
> > >>>> type signature ClusterClient instead of
> > >>>> <ClusterClient & NewClusterClient or even cannot represent the
> > >>>> latter if we aren't in a type variable context. This should not
> affect
> > >>>> anything internal in Flink scope.
> > >>>>
> > >>>> However, as mentioned by Kostas in the JIRA and a previous
> discussion
> > >>>> under a commit[2], it seems that we provide some levels of backward
> > >>>> compatibility for ClusterClient and thus it's better to start a
> public
> > >>>> discussion here.
> > >>>>
> > >>>> There are two concerns from my side.
> > >>>>
> > >>>> 1. How much impact this proposal brings to users programming
> directly
> > >>>> to ClusterClient?
> > >>>>
> > >>>> The specific changes here are add two methods `submitJob` and
> > >>>> `requestJobResult` which are already implemented by
> RestClusterClient
> > >>>> and MiniClusterClient. Users would only be affected if they create
> > >>>> a class that inherits ClusterClient and doesn't implement these
> > >>>> methods. Besides, users who create a class that implements
> > >>>> NewClusterClient would be affected by the removal of
> NewClusterClient.
> > >>>>
> > >>>> If we have to provide backward compatibility and the impact is no
> > >>>> further than those above, we can deprecate NewClusterClient, merge
> > >>>> the methods into ClusterClient with a dummy default like throw
> > >>>> Exception.
> > >>>>
> > >>>> 2. Why do we provide backward compatibility for ClusterClient?
> > >>>>
> > >>>> It already surprises Kostas and me while we think ClusterClient is a
> > >>>> totally internal class which we can evolve regardless of api
> > >>>> stability. Our community promises api stability by marking class
> > >>>> and/or method as @Public/@PublicEvolving. It is wried and even
> > >>>> dangerous we are somehow enforced to provide backward compatibility
> > >>>> for classes without any annotation.
> > >>>>
> > >>>> Besides, as I mention in [2], users who anyway want to program
> > >>>> directly to internal classes/interfaces are considered to prepare to
> > >>>> make adaptations when bump version of Flink.
> > >>>>
> > >>>> Best,
> > >>>> tison.
> > >>>>
> > >>>> [1] https://issues.apache.org/jira/browse/FLINK-14096
> > >>>> [2]
> > >>>>
> > >>>>
> > >>>
> > >>
> >
> https://github.com/apache/flink/commit/dc9e4494dddfed36432e6bbf6cd3231530bc2e01
> > >>>>
> > >>>
> > >>>
> > >>> --
> > >>> Best Regards
> > >>>
> > >>> Jeff Zhang
> > >>>
> > >>
> >
> >
>