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/28 13:30:30 UTC
[GitHub] flink pull request #3633: [FLINK-5982] [runtime] Refactor AbstractInvokable ...
GitHub user tony810430 opened a pull request:
https://github.com/apache/flink/pull/3633
[FLINK-5982] [runtime] Refactor AbstractInvokable and StatefulTask
This PR wants to merge `AbstractInvokable` and `StatefulTask` to eliminate "lazy initialization" in `Task`.
The following are what I did in this PR:
1. Merge `AbstractInvokable` and `StatefulTask` and use constructor to initialize `Environment` and `TaskStateHandles` in `AbstractInvokable`.
2. Update all subclasses and corresponding tests. For now, batch tasks are still not stateful, so I added `IllegalStateException()` in constructor and `UnsupportedOperationException()` in the methods for supporting stateful task.
3. Update the behavior of `AbstractInvokable` in `Task`, including `loadAndInstantiateInvokable()`, `triggerCheckpointBarrier()` and `notifyCheckpointComplete()`.
4. Add tests for modification in `Task` and tests the constructors' behavior for all concrete subclasses which inherit `AbstractInvokable`.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tony810430/flink FLINK-5982
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/3633.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 #3633
----
commit 31748905eaa3b4897f0b02473526b7fa05c2304e
Author: Tony Wei <to...@gmail.com>
Date: 2017-03-15T03:13:41Z
[FLINK-5982] Refactor 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 #3633: [FLINK-5982] [runtime] Refactor AbstractInvokable and Sta...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3633
I finally found the time to look at this.
Have rebased it and applied some simplifications:
- convenience constructors for stateless tasks
- test utils that provide mock environments directly
- Task Test Harnesses create the tasks lazily via a factory.
All in all that makes for a good patch in my opinion.
Will merge this...
---
[GitHub] flink issue #3633: [FLINK-5982] [runtime] Refactor AbstractInvokable and Sta...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3633
Sorry, I did not manage to merge this, yet. Will try to take it on after the 1.3 release.
Thanks for the offer to rebase that. I can ping you when I would be getting close to merging this, so that you could rebase it once then (rather than now and potentially again in a few weeks).
---
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 #3633: [FLINK-5982] [runtime] Refactor AbstractInvokable and Sta...
Posted by tony810430 <gi...@git.apache.org>.
Github user tony810430 commented on the issue:
https://github.com/apache/flink/pull/3633
Hi @StephanEwen
Have you been reviewing this PR? If not, I would like to rebase it and fix those conflicts. What do you think?
---
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 pull request #3633: [FLINK-5982] [runtime] Refactor AbstractInvokable ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/3633
---
[GitHub] flink issue #3633: [FLINK-5982] [runtime] Refactor AbstractInvokable and Sta...
Posted by tony810430 <gi...@git.apache.org>.
Github user tony810430 commented on the issue:
https://github.com/apache/flink/pull/3633
I see. 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 #3633: [FLINK-5982] [runtime] Refactor AbstractInvokable and Sta...
Posted by tony810430 <gi...@git.apache.org>.
Github user tony810430 commented on the issue:
https://github.com/apache/flink/pull/3633
Hi @StephanEwen
Do you have time recently? Or can we find someone to review it if you are too busy to do this? Thank you.
---
[GitHub] flink issue #3633: [FLINK-5982] [runtime] Refactor AbstractInvokable and Sta...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3633
Thanks for this refactoring. I had only a quick look so far, but one thought was that this patch could probably be simplified by making `triggerCheckpoint()` (and restore / notifyComplete) not abstract in the `AbstractInvokable`. Instead let it throw the `UnsupportedOperationException`.
That way, we avoid refactoring all the test classes to implement those methods (which would be nice).
---
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 #3633: [FLINK-5982] [runtime] Refactor AbstractInvokable and Sta...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3633
Thanks, I will try and look at this over the next week.
It is quite a big change, so I might need a bit of time to review it properly...
---
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 #3633: [FLINK-5982] [runtime] Refactor AbstractInvokable and Sta...
Posted by tony810430 <gi...@git.apache.org>.
Github user tony810430 commented on the issue:
https://github.com/apache/flink/pull/3633
Thanks a lot for doing these work. Nice to see this PR be merged. =)
---
[GitHub] flink issue #3633: [FLINK-5982] [runtime] Refactor AbstractInvokable and Sta...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3633
Sorry about the delay. I still have this on my list...
---
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 #3633: [FLINK-5982] [runtime] Refactor AbstractInvokable and Sta...
Posted by tony810430 <gi...@git.apache.org>.
Github user tony810430 commented on the issue:
https://github.com/apache/flink/pull/3633
@StephanEwen Thanks for the suggestion. I have updated the patch.
---
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 #3633: [FLINK-5982] [runtime] Refactor AbstractInvokable and Sta...
Posted by tony810430 <gi...@git.apache.org>.
Github user tony810430 commented on the issue:
https://github.com/apache/flink/pull/3633
Hi @StephanEwen
Do you have time to review this PR recently?
---
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.
---