You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by juliennju <gi...@git.apache.org> on 2017/07/01 10:53:21 UTC
[GitHub] flink pull request #4241: [FLINK-7015] [streaming] separate from OperatorCon...
GitHub user juliennju opened a pull request:
https://github.com/apache/flink/pull/4241
[FLINK-7015] [streaming] separate from OperatorConfig from StreamConfig
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Xupingyong/flink FLINK-7015
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4241.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 #4241
----
commit b35641760276d42e15ec17279404788760c894fc
Author: pingyong.xpy <pi...@alibaba-inc.com>
Date: 2017-07-01T10:47:00Z
[FLINK-7015] [streaming] separate from OperatorConfig from StreamConfig
----
---
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 #4241: [FLINK-7015] [streaming] separate from OperatorConfig fro...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/4241
Hi @juliennju,
thanks for opening this PR and for interest in changing these parts of these code.
However, these changes touch somewhat intricate parts of the code where some other people are working and where also several people have plans and thoughts about how to improve things. Before doing work on these parts I would suggest to start a `[DISCUSS]` thread on the dev mailing list to 1) describe your motivation for changing these parts, 2) see what other people have already thought about this, and 3) to coordinate changes with other people working on this.
I'm sorry for the inconvenience this causes you because you already started working on code.
I'm aware of at least @StefanRRichter, @StephanEwen and me as having spend some cycles thinking about changes in these parts.
For example, we would definitely need to discuss what constitutes "operator config", "task config" and general "stream config" and how we want to shield the different components from each other and make them more modular.
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 issue #4241: [FLINK-7015] [streaming] separate OperatorConfig from Str...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/4241
Also, for actually doing the changes I suggest separate steps, i.e. separate commits. With possibly separate PRs to make reviewing easier and to make the changes more isolated:
- Rename `StreamConfig` to `StreamTaskConfig` and make it serialisable, instead of relying on an underlying `Configuration`. This means that the `StreamTaskConfig` itself has fields for storing settings.
- Introduce `OperatorConfig` and move only those fields that the operator should see from `StreamTaskConfig` to `OperatorConfig`. Initialize the operator with an `OperatorConfig`.
---
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 #4241: [FLINK-7015] [streaming] separate OperatorConfig from Str...
Posted by XuPingyong <gi...@git.apache.org>.
Github user XuPingyong commented on the issue:
https://github.com/apache/flink/pull/4241
Thanks @aljoscha and @StephanEwen, I have updated this PR according to your advice.
- Rename StreamConfig to StreamTaskConfig.
- Introduce serialisable OperatorConfig and move only those fields that are tied to one operator within the chain from StreamTaskConfig to OperatorConfig. Initialize the operator with an OperatorConfig.
- Introduce OperatorContext which is an interface that is a view on some things of OperatorConfig. It is provided for setting up an operator.
---
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 #4241: [FLINK-7015] [streaming] separate OperatorConfig from Str...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4241
In general, it is a good idea to clean that up a bit. That code needed some refactoring.
As @aljoscha pointed out, that is an intricate part, so we need to review that carefully. To be able to review that, we need a good description of what the refactoring did. Trying to understand that from the code alone will take a lot of time and most likely slow down the merging of this by a lot.
I would suggest to write
- what exactly was addressed
- what is the new design
- does it change behavior, or is it only code abstraction internally
As another point: I just took a quick look at the `OperatorConfig`. I think I would be nice to move away from the model of having a Map of "configKey" to values, but just to make this a serializable class with the respective fields. That would be simpler to understand and maintain.
---
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.
---