[GitHub] flink pull request #5881: [FLINK-9190][yarn] fix YarnResourceManager sometim...

[sihuazhou]

Github user sihuazhou commented on a diff in the pull request:

    https://github.com/apache/flink/pull/5881#discussion_r183269424
  
    --- Diff: 
flink-yarn/src/main/java/org/apache/flink/yarn/YarnResourceManager.java ---
    @@ -265,6 +272,29 @@ protected void initialize() throws 
ResourceManagerException {
                }
        }
     
    +   @Override
    +   public CompletableFuture<RegistrationResponse> registerTaskExecutor(
    --- End diff --
    
    I've moved the `ack the pending registration` to `workerStarted()`. but I 
found it still a little chance could lead the registration to be failed...
    
    
![image](https://user-images.githubusercontent.com/7480427/39105198-21a59626-46e7-11e8-809b-d111e81af51b.png)
    
    e.g, if the some unexpected things occur between lines `700` and `720`, 
then the registration can still fail... I think maybe we should override the 
`registerTaskExecutor()` and ack the pending registration only when the future 
of `super.registerTaskExecutor()` is completed, what do you think?



---