You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by tony810430 <gi...@git.apache.org> on 2017/03/04 13:21:39 UTC
[GitHub] flink pull request #3474: [FLINK-4714] [runtime] [streaming] Set task state ...
GitHub user tony810430 opened a pull request:
https://github.com/apache/flink/pull/3474
[FLINK-4714] [runtime] [streaming] Set task state to RUNNING after state has been restored
The changes in this PR are
- add `open()` method in `AbstractInvokable` to let the invokable can be initialized during `DEPLOYING` state in `Task`. The default behavior is do nothing.
- separate `invokable.invoke()` into `invokable.open()` and `invokable.invoke()` in `Task`, and transit task state from `DEPLOYING` to `RUNNING` between them.
- `TaskCanceler` should be used to cancel the invokable in `DEPLOYING` state because invokable might be called in that state now.
- update `StreamTask` and `StreamTaskTestHarness` to solve this issue in FLINK-4714
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tony810430/flink FLINK-4714
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/3474.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #3474
----
commit baa0285ce481db0f96d59ed71d86f81b1f5fcd65
Author: Tony Wei <to...@gmail.com>
Date: 2017-03-03T08:46:22Z
[FLINK-4714] Set task state to RUNNING after state has been restored
----
---
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.
---
[GitHub] flink issue #3474: [FLINK-4714] [runtime] [streaming] Set task state to RUNN...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/3474
The PR does not contain any tests which are important for these changes. I'll review your code once you've added tests.
---
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.
---
[GitHub] flink issue #3474: [FLINK-4714] [runtime] [streaming] Set task state to RUNN...
Posted by tony810430 <gi...@git.apache.org>.
Github user tony810430 commented on the issue:
https://github.com/apache/flink/pull/3474
OK, I will add some tests to verify the correctness of these changes. Thank you.
---
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.
---
[GitHub] flink issue #3474: [FLINK-4714] [runtime] [streaming] Set task state to RUNN...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/3474
+1 for the described changes :-)
---
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.
---
[GitHub] flink issue #3474: [FLINK-4714] [runtime] [streaming] Set task state to RUNN...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3474
Yes, we should update all subclasses with the one-argument constructor. If the `AbstractInvokable` does not have a zero-argument constructor any more, all subclasses will be forced to call the one-argument constructor.
---
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.
---
[GitHub] flink issue #3474: [FLINK-4714] [runtime] [streaming] Set task state to RUNN...
Posted by tony810430 <gi...@git.apache.org>.
Github user tony810430 commented on the issue:
https://github.com/apache/flink/pull/3474
@StephanEwen
How about using `AbstractInvokable.class.isAssignableFrom()` and `StatefulTask.class.isAssignableFrom()` to check the loaded class?
Then, we can check if the `TaskStateHandles` is null out of constructor and call different constructor for two type of class.
---
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.
---
[GitHub] flink issue #3474: [FLINK-4714] [runtime] [streaming] Set task state to RUNN...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3474
Yes, the `setEnvironment()` method will not be needed any more.
---
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.
---
[GitHub] flink issue #3474: [FLINK-4714] [runtime] [streaming] Set task state to RUNN...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3474
I would suggest to do the change differently. Right now, all the task logic is pretty complex as it is, and this makes it even more complex.
My suggestion would be to
1. Change the `AbstractInvokable` to not have a zero-argument constructor, but only one which takes an `Environment`
2. Change the creation of the invokables to call that constructor, update all the tests
3. Change the `StreamTask` to do its initialization of everything in the constructor. That way, we also avoid all the `NullPointerException`
That way, transitioning to `RUNNING` just before calling `invoke()` will fix the issue.
If you want to follow up on this, I would suggest to make individual pull requests for (1,2) and (3)
---
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.
---
[GitHub] flink issue #3474: [FLINK-4714] [runtime] [streaming] Set task state to RUNN...
Posted by tony810430 <gi...@git.apache.org>.
Github user tony810430 commented on the issue:
https://github.com/apache/flink/pull/3474
Hi @StephanEwen
This suggestion seems to use constructor to set `Environment` instead of using `setEnvironment(env)` method and may not need it any more.
Use the constructor to initialize other setting after `Environment` has been set up if those are needed to be set up before `invoke()`. For example: `StreamTask`.
If I misunderstand anything, please correct me. Thank you.
---
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.
---
[GitHub] flink issue #3474: [FLINK-4714] [runtime] [streaming] Set task state to RUNN...
Posted by tony810430 <gi...@git.apache.org>.
Github user tony810430 commented on the issue:
https://github.com/apache/flink/pull/3474
Thank all of you for your help.
I will block this PR for now, and refresh it once I have finished refactoring `AbstractInvokable` and `StatefulTask`.
---
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.
---
[GitHub] flink issue #3474: [FLINK-4714] [runtime] [streaming] Set task state to RUNN...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3474
@tony810430 I think we can drop the interface `StatefulTask` and just assume every task is stateful. The `BatchTask` can throw an error, if it gets a non-null value for the state in the constructor.
---
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.
---
[GitHub] flink issue #3474: [FLINK-4714] [runtime] [streaming] Set task state to RUNN...
Posted by tony810430 <gi...@git.apache.org>.
Github user tony810430 commented on the issue:
https://github.com/apache/flink/pull/3474
Hi @StephanEwen
I think this approach is cleaner than I do.
However, what I am concerned is that `setEnvironment()` method can be inherited by `AbstractInvokable`'s subclass but constructor can't. It means all subclasses need to implement this constructor and that is not enforced to do so.
Although we can update all subclasses with an one-argument constructor which takes `env` only, I think it is not safer than setting `env` by calling a final method.
What do you think? If there is any mistake in my words, please tell me as well. Thank you.
---
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.
---
[GitHub] flink issue #3474: [FLINK-4714] [runtime] [streaming] Set task state to RUNN...
Posted by tony810430 <gi...@git.apache.org>.
Github user tony810430 commented on the issue:
https://github.com/apache/flink/pull/3474
Hi, @StephanEwen
I mean although all subclasses will be forced to call the one-argument constructor, it doesn't force all subclasses need to have the one-argument constructor. It seems not intuition to add an one-argument constructor if there is another requirement for a new subclass.
However, tests might help us find out this mistake. Maybe it is not a problem.
One more question, `StreamTask` needs `setInitialState()` to initialize itself as well. Will that be okey to make the subclass of both `AbstractInvokable` and `StatefulTask` to have two-arguments constructor and remove `setInitialState()` method?
If it works fine for you and @tillrohrmann, I have no more question.
Please help me create a JIRA task for (1, 2). I will pend this PR until I have finished that task and redo it as (3). Thank you.
---
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.
---
[GitHub] flink issue #3474: [FLINK-4714] [runtime] [streaming] Set task state to RUNN...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3474
@tony810430 That's a good idea, let's have a two argument constructor with `Environment` and `TaskStateHandles `.
The `AbstractInvokable` and its subclasses are a very low level internal API. It is okay if we make this a "contract" that they need this constructor - runtime will fail the task otherwise.
In fact, there are only two main implementations of the `AbstractInvokable`: ``StreamTask` and `BatchTask`, with some subclasses. That is okay to manage.
The benefit of having eager initialization of fields and state is much higher than the burden to add the constructor.
---
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.
---
[GitHub] flink issue #3474: [FLINK-4714] [runtime] [streaming] Set task state to RUNN...
Posted by tony810430 <gi...@git.apache.org>.
Github user tony810430 commented on the issue:
https://github.com/apache/flink/pull/3474
@StephanEwen
BTW, I have opened a JIRA task for refactoring `AbstractInvokable` and `StatefulTask`. How about discussing this topic in [FLINK-5982](https://issues.apache.org/jira/browse/FLINK-5982)?
---
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.
---