Thanks for your suggestions and information. I'll therefore reopen the corresponding pull request.
Best, tison. Till Rohrmann <trohrm...@apache.org> 于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 <aljos...@apache.org> > 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 <wander4...@gmail.com> 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 <shixiaoga...@gmail.com> 于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 <zjf...@gmail.com> 于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 <wander4...@gmail.com> 于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 > > >>> > > >> > > > > >
