You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by aljoscha <gi...@git.apache.org> on 2016/01/15 18:40:11 UTC
[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...
GitHub user aljoscha opened a pull request:
https://github.com/apache/flink/pull/1513
[FLINK-3243] Fix Interplay of TimeCharacteristic and Time Windows
This adds dedicated WindowAssigners for processing time and event time.
timeWindow() and timeWindowAll() respect the TimeCharacteristic set
on the StreamExecutionEnvironment.
This will make the easy stuff easy, i.e. using time windows and quickly
switching the time characteristic. Users will then have the flexibility
to mix different kinds of window assigners in their job.
This also expands the translation tests to verify that the correct
window operators are instantiated.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aljoscha/flink time-characteristic-fix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/1513.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 #1513
----
commit 7df1f2265beec91584373ac66f8cdc05fc46e6f8
Author: Aljoscha Krettek <al...@gmail.com>
Date: 2016-01-15T15:37:16Z
[hotfix] Fix enum names of TimeCharacteristic
Before, they where CamelCase while standard Java style suggests that
they should be ALL_UPPERCASE.
commit f5e0a9d86cdf8f292ca0c4a45d34abb98f3ee68b
Author: Aljoscha Krettek <al...@gmail.com>
Date: 2016-01-15T16:05:12Z
[FLINK-3243] Fix Interplay of TimeCharacteristic and Time Windows
This adds dedicated WindowAssigners for processing time and event time.
timeWindow() and timeWindowAll() respect the TimeCharacteristic set
on the StreamExecutionEnvironment.
This will make the easy stuff easy, i.e. using time windows and quickly
switching the time characteristic. Users will then have the flexibility
to mix different kinds of window assigners in their job.
This also expands the translation tests to verify that the correct
window operators are instantiated.
----
---
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: [FLINK-3243] Fix Interplay of TimeCharacterist...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/1513#issuecomment-172574417
I adapted the documentation.
---
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: [FLINK-3243] Fix Interplay of TimeCharacterist...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha closed the pull request at:
https://github.com/apache/flink/pull/1513
---
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: [FLINK-3243] Fix Interplay of TimeCharacterist...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/1513#issuecomment-181810307
I rebased it to master and updated.
---
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: [FLINK-3243] Fix Interplay of TimeCharacterist...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/1513#issuecomment-183326898
Any more comments? Ideas? I think we should merge this soon because it should go into the 1.0 release.
---
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: [FLINK-3243] Fix Interplay of TimeCharacterist...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/1513#discussion_r52184878
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/TimeCharacteristic.java ---
@@ -34,7 +34,7 @@
* because the contents of the windows depends on the speed in which elements arrive. It is, however,
* the cheapest method of forming windows and the method that introduces the least latency.
*/
- ProcessingTime,
+ PROCESSING_TIME,
--- End diff --
+1 for standard java conventions 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: [FLINK-3243] Fix Interplay of TimeCharacterist...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/1513#discussion_r52188674
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/KeyedStream.java ---
@@ -163,7 +166,11 @@ public KeyedStream(DataStream<T> dataStream, KeySelector<T, KEY> keySelector, Ty
* @param size The size of the window.
*/
public WindowedStream<T, KEY, TimeWindow> timeWindow(AbstractTime size) {
- return window(TumblingTimeWindows.of(size));
+ if (environment.getStreamTimeCharacteristic() == TimeCharacteristic.PROCESSING_TIME) {
--- End diff --
Yes, but I think this is inevitable because in the Streaming API we don't (yet) have deferred creation of the actual runtime operator graph from the graph of API operations as we have in the batch API.
I think there are some other cases where the assumption about the `ExecutionConfig` also break.
---
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: [FLINK-3243] Fix Interplay of TimeCharacterist...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/1513#issuecomment-181777122
You're right, I'm changing it. But it was also me who didn't notice when we put it in initially :sweat_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: [FLINK-3243] Fix Interplay of TimeCharacterist...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1513#issuecomment-181511999
Is the switch from `ProcessingTime` to `PROCESSING_TIME` actually necessary?
Breaks yet one more thing for users just because of a matter of taste.
There are only so many breaking changes that we can do without upsetting people too much, and I would like to make those where it makes a difference.
---
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: [FLINK-3243] Fix Interplay of TimeCharacterist...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/1513#issuecomment-181557496
IMO, it is not a question of taste but a question of trying to make the code base adhere to Java coding standards.
I can roll back that change if we want this, of course.
---
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: [FLINK-3243] Fix Interplay of TimeCharacterist...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/1513#discussion_r52185124
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/KeyedStream.java ---
@@ -163,7 +166,11 @@ public KeyedStream(DataStream<T> dataStream, KeySelector<T, KEY> keySelector, Ty
* @param size The size of the window.
*/
public WindowedStream<T, KEY, TimeWindow> timeWindow(AbstractTime size) {
- return window(TumblingTimeWindows.of(size));
+ if (environment.getStreamTimeCharacteristic() == TimeCharacteristic.PROCESSING_TIME) {
--- End diff --
I'm not sure how the rest of the DataStream API is behaving, but I though that it doesn't matter when the environment settings (for example parallelism) are set.
In this case, users have to set the `TimeCharacteristic` before using operators.
---
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: [FLINK-3243] Fix Interplay of TimeCharacterist...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/1513#issuecomment-172567227
Right as always. I'm changing the docs now.
---
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: [FLINK-3243] Fix Interplay of TimeCharacterist...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/1513#issuecomment-184715239
@rmetzger @StephanEwen Do you still have objections? I think this should go in ASAP.
---
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: [FLINK-3243] Fix Interplay of TimeCharacterist...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/1513#issuecomment-184196790
I rebased on top of current master.
Btw, still still has the issue that the time characteristic only applies to window operations that are created after setting the time characteristic. Changing the time characteristic does not affect already created window operations.
---
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: [FLINK-3243] Fix Interplay of TimeCharacterist...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/1513#issuecomment-185132200
Manually 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: [FLINK-3243] Fix Interplay of TimeCharacterist...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1513#issuecomment-184875475
Looks good to me, +1 to merge this
---
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: [FLINK-3243] Fix Interplay of TimeCharacterist...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1513#issuecomment-181776065
For new classes, it makes sense. Was a mistake on my end to name them like this in the first place.
But users that adopted this draw in my experience more satisfaction from stable code than from a style nuance.
---
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: [FLINK-3243] Fix Interplay of TimeCharacterist...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1513#issuecomment-172533529
I think the documentation hasn't been adopted to the 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.
---