[GitHub] TisonKun commented on issue #6729: [FLINK-10386] [taskmanager] Remove legacy class TaskExecutionStateListener
TisonKun commented on issue #6729: [FLINK-10386] [taskmanager] Remove legacy class TaskExecutionStateListener URL: https://github.com/apache/flink/pull/6729#issuecomment-427768967 @tillrohrmann sounds good. As you suggest, we can replace `notifyFinalState` with `updateTaskExecutionState` since there is no different from API view and a `updateTaskExecutionState` without final state seems wired. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] TisonKun commented on issue #6729: [FLINK-10386] [taskmanager] Remove legacy class TaskExecutionStateListener
TisonKun commented on issue #6729: [FLINK-10386] [taskmanager] Remove legacy class TaskExecutionStateListener URL: https://github.com/apache/flink/pull/6729#issuecomment-427761218 @tillrohrmann thanks for your review! This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] TisonKun commented on issue #6729: [FLINK-10386] [taskmanager] Remove legacy class TaskExecutionStateListener
TisonKun commented on issue #6729: [FLINK-10386] [taskmanager] Remove legacy class TaskExecutionStateListener URL: https://github.com/apache/flink/pull/6729#issuecomment-427071700 All "Why are we removing these assertions?" is as you say, we do not send those state updates via `TaskManagerActions.updateTaskExecutionState`. Maybe we can verify `TaskManagerActions.notifyFatalError`/`TaskManagerActions.failTask`/`TaskManagerActions.notifyFinalState` but all of them don't include detail task state. Besides, as the communication with @Clark , `TaskTest` is a test based on legacy mode, there is a pull request #6778 to port it to FLIP-6 code base. I think the main issue is whether we replace `notifyObservers` with `TaskManagerActions.updateTaskExecutionState`. My opinion is not for now to hold the current(FLIP-6) message flow and defer the decision later. If so, we remove those assertion because we don't send those message and the assertion should not hold. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] TisonKun commented on issue #6729: [FLINK-10386] [taskmanager] Remove legacy class TaskExecutionStateListener
TisonKun commented on issue #6729: [FLINK-10386] [taskmanager] Remove legacy class TaskExecutionStateListener URL: https://github.com/apache/flink/pull/6729#issuecomment-427068734 @tillrohrmann currently our production codes never deal with the message send by `notifyObservers`, because the only class deal with this message is `ActorGatewayTaskExecutionStateListener` who is only used in `TaskTest`. In other word, these message are never sent to `JobMaster`. So replace them with `TaskManagerActions.updateTaskExecutionState` increase messages sent to `JobMaster`. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] TisonKun commented on issue #6729: [FLINK-10386] [taskmanager] Remove legacy class TaskExecutionStateListener
TisonKun commented on issue #6729: [FLINK-10386] [taskmanager] Remove legacy class TaskExecutionStateListener URL: https://github.com/apache/flink/pull/6729#issuecomment-423867964 cc @Clark @GJL @tillrohrmann This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] TisonKun commented on issue #6729: [FLINK-10386] [taskmanager] Remove legacy class TaskExecutionStateListener
TisonKun commented on issue #6729: [FLINK-10386] [taskmanager] Remove legacy class TaskExecutionStateListener URL: https://github.com/apache/flink/pull/6729#issuecomment-423611672 cc @StefanRRichter @zentol This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] TisonKun commented on issue #6729: [FLINK-10386] [taskmanager] Remove legacy class TaskExecutionStateListener
TisonKun commented on issue #6729: [FLINK-10386] [taskmanager] Remove legacy class TaskExecutionStateListener URL: https://github.com/apache/flink/pull/6729#issuecomment-423436864 Travis fails on `TaskTest` because it is based on legacy code base. Temporarily hack to refine it. Note that although it is a hack, the coverage of test is kept -- failure messages are checked by previous assertion. `TaskTest` based on FLIP-6 would be a follow-up pull request and make this one as clean as possible. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
