You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by gyfora <gi...@git.apache.org> on 2015/04/13 18:01:05 UTC
[GitHub] flink pull request: Stream graph refactor
GitHub user gyfora opened a pull request:
https://github.com/apache/flink/pull/594
Stream graph refactor
This is a refactor for the internal representation of the streaming topologies.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mbalassi/flink FLINK-1876
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/594.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 #594
----
commit d21941daa6eb8ff6188ab501bb633b36e60401f2
Author: Gyula Fora <gy...@apache.org>
Date: 2015-04-13T09:21:37Z
[FLINK-1866] [streaming] StreamConfig key bugfix
commit 17ab48117fd62010dcda0018fc7c4c520eb86327
Author: Gyula Fora <gy...@apache.org>
Date: 2015-04-13T14:00:46Z
[FLINK-1876] [streaming] StreamGraph refactor
----
---
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: Stream graph + internal refactor
Posted by gyfora <gi...@git.apache.org>.
Github user gyfora commented on the pull request:
https://github.com/apache/flink/pull/594#issuecomment-93083350
I fixed all the comments, except for the SingleOutputStreamOperator thing. Let's leave that for later :D
---
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: Stream graph + internal refactor
Posted by gyfora <gi...@git.apache.org>.
Github user gyfora commented on the pull request:
https://github.com/apache/flink/pull/594#issuecomment-93652334
It is already merged.
---
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: Stream graph + internal refactor
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/594#issuecomment-92860931
Also, there are packages streamgraph and streamoperator which could be renamed to graph and operators, to improve consistency.
Further, there are two concepts we call operators here, this might lead to confusion. There are the actual operator implementations in streamoperator and there are things like SingleOutputStreamOperator and StreamCrossOperator. I know that the batch API is also very messy in naming these things but maybe we can find better names for the streaming API while we still have a chance to change things. :smile:
---
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: Stream graph + internal refactor
Posted by gyfora <gi...@git.apache.org>.
Github user gyfora commented on the pull request:
https://github.com/apache/flink/pull/594#issuecomment-93653375
Haha no problem. I will make sure it is more reviewable in the future PRs.
---
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: Stream graph + internal refactor
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/594#issuecomment-93270919
+1 for merge
On Apr 15, 2015 10:44 AM, "Gyula Fora" <no...@github.com> wrote:
> I will merge this later today if no objections .
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/flink/pull/594#issuecomment-93264219>.
>
---
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: Stream graph + internal refactor
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/594
---
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: Stream graph + internal refactor
Posted by gyfora <gi...@git.apache.org>.
Github user gyfora commented on the pull request:
https://github.com/apache/flink/pull/594#issuecomment-93264219
I will merge this later today if no objections .
---
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: Stream graph + internal refactor
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/594#issuecomment-92854325
Regarding StreamRecord, is the UID still required? In my understanding we are working towards state-snapshotting, so that would not require to have IDs in records anymore, correct? I mentioned at some earlier point that the IDs are not used anymore, so what's the status now?
If we got rid of the UID then we can also get rid of StreamRecord and StreamRecord serializer.
---
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: Stream graph + internal refactor
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/594#issuecomment-92862631
Package api.function and its sub packages should be pluralised for consistency.
---
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: Stream graph + internal refactor
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/594#issuecomment-92857115
Package org.apache.flink.streaming.api.datastream.temporaloperator could be renamed to org.apache.flink.streaming.api.datastream.temporal, the former seems to be quite a mouthful.
---
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: Stream graph + internal refactor
Posted by hsaputra <gi...@git.apache.org>.
Github user hsaputra commented on the pull request:
https://github.com/apache/flink/pull/594#issuecomment-93653085
Ah yes, it should be fine. I should have replied this as more generic comment for future PRs.
Sorry if I confused you @gyfora
---
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: Stream graph + internal refactor
Posted by hsaputra <gi...@git.apache.org>.
Github user hsaputra commented on the pull request:
https://github.com/apache/flink/pull/594#issuecomment-93635634
This is great patch, but as @mxm mentioned before, it almost impossible to review patches with this size.
If the PR could not be chopped into separate PRs, lets try to have smaller commits to help. Before merge we could squash it to make it just one commit.
---
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: Stream graph + internal refactor
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/594#issuecomment-92856455
By the way, all these comments I'm making. I'm just making them to keep track of things. If we discuss them and want to implement some changes I can also do this myself, just want to get opinions here.
---
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: Stream graph + internal refactor
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/594#issuecomment-93080706
Apart from these naming issues I really like the idea behind the streaming API. :smile:
---
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: Stream graph + internal refactor
Posted by mbalassi <gi...@git.apache.org>.
Github user mbalassi commented on the pull request:
https://github.com/apache/flink/pull/594#issuecomment-92820251
By the way scrolling through the code: Big :+1: for the `streamoperator` package and for the `StreamNode`.
---
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: Stream graph + internal refactor
Posted by mbalassi <gi...@git.apache.org>.
Github user mbalassi commented on the pull request:
https://github.com/apache/flink/pull/594#issuecomment-92817866
When merging two things to look out for: I have already pushed the key bugfix for the StreamConfig and have have to make sure to not only close this PR but also #508.
---
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: Stream graph + internal refactor
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/594#issuecomment-92856051
Package org.apache.flink.streaming.runtime.streamtask could be renamed to org.apache.flink.streaming.runtime.tasks, the streaming part is already clear from *.streaming.* in the package prefix. io and partitioner are also not called streamio and streampartitioner. This would make things more homogenous.
---
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: Stream graph + internal refactor
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/594#issuecomment-92780564
This is a huge PR but from what I've seen you made very sensible 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.
---