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