[GitHub] flink issue #2353: [FLINK-4355] [cluster management] Implement TaskManager s...

[tillrohrmann <gi...@git.apache.org>]

Github user tillrohrmann commented on the issue:

    https://github.com/apache/flink/pull/2353
  
    Really good code @StephanEwen :-) I like it a lot that you've pulled out the registration logic from the `TaskExecutor`. This makes it much cleaner and easier to test.
    
    I'm actually wondering whether we can actually take it a step further. What I've noticed is that we have to pass the `TaskExecutor` to different places to have access to the its state and to run things in the main thread context. I'm not sure whether this is really necessary. 
    
    The registration information should be more or less constant and the main thing which has to happen in the main thread context is the setting of the connected gateway. So wouldn't it be possible that the `ResourceManagerRegistration` is something like a cancelable future which is instantiated with all necessary information. It performs the registration logic completely in the background and once it is completed we can create a `TaskExecutorToResourceManagerConnection` in the main thread context. Whenever we have to start a new registration, we cancel the future (if there is still another registration running).
    
    Pulling out the `TaskExecutor` from the registration process has the advantage that the components are even further separated and, thus, easier to test.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---