[GitHub] flink issue #6224: [FLINK-9687]Delay the state fetch only when the triggerRe...
Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/6224 I merged it ð ---
[GitHub] flink issue #6224: [FLINK-9687]Delay the state fetch only when the triggerRe...
Github user Aitozi commented on the issue: https://github.com/apache/flink/pull/6224 OK ð ---
[GitHub] flink issue #6224: [FLINK-9687]Delay the state fetch only when the triggerRe...
Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/6224 @Aitozi Is it ok for you that I merge it but rename to FLINK-5363 and write in the commit message that it also resolves FLINK-9687? I also assigned you to both of these issues. ---
[GitHub] flink issue #6224: [FLINK-9687]Delay the state fetch only when the triggerRe...
Github user Aitozi commented on the issue: https://github.com/apache/flink/pull/6224 Aha, I meat to improve the performance of windowOperator, In the scenario mentioned in the issue, this PR can avoid that bug, lucky hit ;-) ---
[GitHub] flink issue #6224: [FLINK-9687]Delay the state fetch only when the triggerRe...
Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/6224 @Aitozi Please see my comment here: https://issues.apache.org/jira/browse/FLINK-9668. I now think we should merge this after all. ---
[GitHub] flink issue #6224: [FLINK-9687]Delay the state fetch only when the triggerRe...
Github user Aitozi commented on the issue: https://github.com/apache/flink/pull/6224 Got it, Thanks for your explanation ;-ï¼. I will close this PR. ---
[GitHub] flink issue #6224: [FLINK-9687]Delay the state fetch only when the triggerRe...
Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/6224 Unfortunately, we cannot distinguish between cleanup timers and other timers. In fact, in the case where we have `allowed lateness = 0` the one timer is both a normal timer and a cleanup timer. ---
[GitHub] flink issue #6224: [FLINK-9687]Delay the state fetch only when the triggerRe...
Github user Aitozi commented on the issue: https://github.com/apache/flink/pull/6224 Get it, this change indeed will lead to the situation you mentioned. > What are the cases where this change leads to an improvement I just read code and think the order is not suitable. > I think it rarely happens that a timer fires while a window is empty I think here is not the one in fact, because we get the window content each time before get the trigger result. AFAIK in there is `cleanupTimer` and `fireTimer` in `internalTimerService` in `windowOperator`. we only have to get window content for `fireTimer` of a window. In now implementation, we have to extract both for the `cleanupTimer` and `fireTimer`. ---
[GitHub] flink issue #6224: [FLINK-9687]Delay the state fetch only when the triggerRe...
Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/6224 No, the motivation was different but the effect of the change is the same, I think. With this change, you can get into a situation where `Trigger.onEventTime()` returns `FIRE` and there is actually no window contents to fire. I think this can potentially be confusing for users that write custom triggers. I'm not saying that this behaviour is bad, I'm only saying that it changes the behaviour, and this can be tricky. This was also the main reason why we didn't change it back when I did the PR. If I remember correctly. What are the cases where this change leads to an improvement? I think it rarely happens that a timer fires while a window is empty, so I think we read the window state in almost all cases when a timer fires. ---
[GitHub] flink issue #6224: [FLINK-9687]Delay the state fetch only when the triggerRe...
Github user Aitozi commented on the issue: https://github.com/apache/flink/pull/6224 Hi, @aljoscha I read the PR of https://issues.apache.org/jira/browse/FLINK-5363, i think it is not a same thing with this PR. You meant to get `TriggerResult` no matter the window content is empty or not. And I meant to check the `TriggerResult` first to avoid get window state when the trigger are not ready to `FIRE`. And I think the cost of get window state is much more cost, so i think we can get the triggerResult first. ---
[GitHub] flink issue #6224: [FLINK-9687]Delay the state fetch only when the triggerRe...
Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/6224 I think the reasons at that time where: - it changes the semantics of Triggers - You can get into a situation where you call `Trigger.onEventTime()`, the `Trigger` returns `FIRE` or `FIRE_AND_PURGE`, and then you don't actually have any window contents to emit ---
[GitHub] flink issue #6224: [FLINK-9687]Delay the state fetch only when the triggerRe...
Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/6224 I had a PR that changed the behaviour for https://issues.apache.org/jira/browse/FLINK-5363. Back then, @StephanEwen and I discussed it, I don't remember the exact discussion anymore but I think we didn't want to change this because it changes the semantics of Triggers. I'm also unsure now about whether we should/can change this. Let me think a bit. ---
[GitHub] flink issue #6224: [FLINK-9687]Delay the state fetch only when the triggerRe...
Github user Aitozi commented on the issue: https://github.com/apache/flink/pull/6224 ping @aljoscha , could you help review it, I'd like to hear your opinion on this PR too. thx ---
[GitHub] flink issue #6224: [FLINK-9687]Delay the state fetch only when the triggerRe...
Github user Aitozi commented on the issue: https://github.com/apache/flink/pull/6224 The failed info in travis error shows the test with `checkClusterEmpty` is wrong, Is this happened due to the reuse of yarn cluster? It seems unrelated with this pull request. ---
