[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...

[aljoscha]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...

[aljoscha]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...

[StephanEwen]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...

[aljoscha]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...

[aljoscha]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...

[aljoscha]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...

[aljoscha]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...

[aljoscha]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...

[StephanEwen]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...

[rmetzger]

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 dataStream, 
KeySelector keySelector, Ty
 * @param size The size of the window.
 */
public WindowedStream 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...

[rmetzger]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...

[aljoscha]

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 dataStream, 
KeySelector keySelector, Ty
 * @param size The size of the window.
 */
public WindowedStream 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...

[StephanEwen]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...

[aljoscha]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...

[rmetzger]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...

[aljoscha]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...

[aljoscha]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-3243] Fix Interplay of TimeCharacterist...

[aljoscha]

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 
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 
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---