je-ik commented on a change in pull request #12759:
URL: https://github.com/apache/beam/pull/12759#discussion_r482255711
##########
File path:
runners/flink/src/main/java/org/apache/beam/runners/flink/translation/wrappers/streaming/DoFnOperator.java
##########
@@ -1330,6 +1338,19 @@ public void setTimer(
@Deprecated
@Override
public void setTimer(TimerData timer) {
+ if (timer.getTimestamp().isAfter(GlobalWindow.INSTANCE.maxTimestamp())) {
Review comment:
I understand the concerns about code duplication. But the duplication
would be really very small. On the other hand - I would say, that the
CleanupTimer abstraction is there precisely for the reasons needed here - the
StatefulDoFnRunner delegates on CleanupTimer the decision to setup a timer for
window. Precisely what we are looking for here. The approach with tweaking
setupTimer internally can have unexpected side-effects in the future, because
there might (in theory) be another timer set after the end of global window.
On the other hand, because the probability of these unwanted side-effects
seem to be low and they would be very much likely caught early in development,
I think we can leave it as it is, although my personal preference would
definitely be the CleanupTimer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
