For any PR that depends on the Client, it would be nice to merge it after the client code was updated...
On Fri, Jul 17, 2015 at 4:32 PM, Matthias J. Sax < [email protected]> wrote: > I can take care of open cancel PR. What is the workflow? Fork the > branch, apply changes, open new PR? > > The stop PR is waiting for review already: > https://github.com/apache/flink/pull/750 > > Not sure if "cancel + stop" should be merged before or after "session" > PR. You should know better which changes are easier to rebase. > > What about PR https://github.com/apache/flink/pull/904 > It also applies changes to WebClient. How does it effect (or is effected > by) all those changes? > > > I agree, that unifying client code should be the last step. > > > -Matthias > > On 07/17/2015 04:18 PM, Stephan Ewen wrote: > > How about this: > > > > - I think we should not block on the "cancel" pull request > > https://github.com/apache/flink/pull/642 > > It is not complex and can be easily forward fitted > > > > - Let's merge the Client "session" pull request soon. > > https://github.com/apache/flink/pull/858 > > It changes the assumptions of the client (the client is job > independent > > and only a gateway to send jobs and trigger actions). > > > > - After that we can in parallel continue with the "stop" pull request > > (not too much logic in the client) and the CLI / Client consolidation. > > > > - The CLI / Client consolidation should most importantly move the > "list" > > and "cancel" code to the client. > > > > Makes sense? > > > > For an approximate time line: > > > > The session pull request should be merged soon. IMHO, Max or me should > make > > a final pass and then sync to add this. I hope it is not more than a few > > days. > > The pull request is a bit delicate, as the session idea changes the > > interaction of client and JobManager a bit, so we'd very much like to get > > this really right. > > > > Greetings, > > Stephan > > > > > > On Fri, Jul 17, 2015 at 2:47 PM, Maximilian Michels <[email protected]> > wrote: > > > >> I'm also in favor of restructuring the Client. Some of this work has > >> already been undergone in this pull request: > >> https://github.com/apache/flink/pull/858 > >> > >> It would be great if we could sync such that we do the restructuring > once > >> the pull request has been merged. We can probably get it in next week. > >> > >> On Fri, Jul 17, 2015 at 1:52 PM, Aljoscha Krettek <[email protected]> > >> wrote: > >> > >>> +1 > >>> Very good idea > >>> > >>> > >>> On Thu, 16 Jul 2015 at 17:57 Fabian Hueske <[email protected]> wrote: > >>> > >>>> Yes definitely. > >>>> The client and submission code is spread out over multiple classes and > >>>> different clients follow different paths. This is a bit messy right > >> now, > >>>> IMO. > >>>> A big +1 to unify and restructure the client. > >>>> > >>>> 2015-07-16 17:52 GMT+02:00 Till Rohrmann <[email protected]>: > >>>> > >>>>> I like the idea to have a single point of access. That would improve > >>>>> maintainability and makes the code easier to understand. Thus +1. > >>>>> > >>>>> On Thu, Jul 16, 2015 at 4:45 PM, Matthias J. Sax < > >>>>> [email protected]> wrote: > >>>>> > >>>>>> Hi, > >>>>>> > >>>>>> I just had a look into CliFrontend and Client and it seems to me, > >>> that > >>>>>> there is no uniform design. > >>>>>> > >>>>>> For example, CliFrontend uses Client to execute "run" and "info" > >>>>>> command. However, for "cancel" and "list" it does not (because > >>>>>> org.apache.flink.client.program.Client) lacks those methods. > >>>>>> > >>>>>> I would recommend to extend Client and adapt CliFrontend. > >>>>>> > >>>>>> There is already a pull request for "cancel": > >>>>>> https://github.com/apache/flink/pull/642 > >>>>>> However, this PR does not adapt CliFrontend and I am not sure if it > >>>> will > >>>>>> be finished at all. A three week old discussion resulted in no > >>>> progress. > >>>>>> > >>>>>> In the current pull request for the new STOP signal, there is also > >>> some > >>>>>> mess with this regard. CliFrontend does not use Client.stop() -> I > >>> will > >>>>>> update this soon (this issues was the trigger to discover this > >>> "mess"), > >>>>>> or include those changes into this work (if we start it). > >>>>>> > >>>>>> Additionally, we might want to uniform WebClient and job manager > >>>>>> WebFrontend, too. I have an open PR that changed WebClient to use > >>>>>> CliFrontend to avoid code duplication. But now, this seems not the > >>>> right > >>>>>> way to go. JobManagerInfoServlet duplicate this code, too. > >>>>>> > >>>>>> I think Client should be the unique class that offers methods for > >> all > >>>>>> request and is used by all other clients. > >>>>>> > >>>>>> What do you think about this? > >>>>>> > >>>>>> > >>>>>> -Matthias > >>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > > >
