flink-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthias J. Sax" <mj...@informatik.hu-berlin.de>
Subject Re: [DISCUSS] Unifying client code
Date Fri, 17 Jul 2015 15:29:23 GMT
Sorry, but I can't follow. What do you mean by "depends on Client"?

Client.java is updated in PR#642 and PR#750 (adding cancel and stop)
PR#750 introduces STOP after all.

There is also PR#917 and PR#858 both working on session IDs, and both
changing Client.java

So all "depend on Client"...

-Matthias



On 07/17/2015 04:54 PM, Stephan Ewen wrote:
> 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 <
> mjsax@informatik.hu-berlin.de> 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 <mxm@apache.org>
>> 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 <aljoscha@apache.org>
>>>> wrote:
>>>>
>>>>> +1
>>>>> Very good idea
>>>>>
>>>>>
>>>>> On Thu, 16 Jul 2015 at 17:57 Fabian Hueske <fhueske@gmail.com>
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 <trohrmann@apache.org>:
>>>>>>
>>>>>>> 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 <
>>>>>>> mjsax@informatik.hu-berlin.de> 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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>>
> 


Mime
View raw message