scwhittle commented on a change in pull request #13032:
URL: https://github.com/apache/beam/pull/13032#discussion_r503470326
##########
File path:
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/SimpleParDoFn.java
##########
@@ -486,14 +487,23 @@ private void processTimers(
for (W window : windowsToCleanup) {
// The stepContext is the thing that know if it is batch or streaming,
hence
// whether state needs to be cleaned up or will simply be discarded so
the
- // timer can be ignored
-
+ // timer can be ignored.
Instant cleanupTime = earliestAllowableCleanupTime(window,
windowingStrategy);
- // if DoFn has OnWindowExpiration then set holds for system timer.
- Instant cleanupOutputTimestamp =
- fnSignature.onWindowExpiration() == null ? cleanupTime :
cleanupTime.minus(1L);
- stepContext.setStateCleanupTimer(
- CLEANUP_TIMER_ID, window, windowCoder, cleanupTime,
cleanupOutputTimestamp);
+ // Set a cleanup timer for state at the end of the window to trigger
onWindowExpiration and
+ // garbage collect state. We avoid doing this for the global window if
there is no window
+ // expiration set as the state will be up when the pipeline terminates.
Setting the timer
+ // leads to a unbounded growth of timers for pipelines with many unique
keys in the global
+ // window.
+ if (cleanupTime.isBefore(GlobalWindow.INSTANCE.maxTimestamp())
Review comment:
I'm not sure I understand your suggestion.
If we eliminate this condition, we are back to always setting timers which
ends up accumulating timers for the global window. If we eliminate setting the
timer if the user did not specify onWindowExpiration, we are removing the
automatic cleanup of windowed user-state for the non global window.
Are you are concerned about setting the timers if onWindowExpiration was
requested for the global window? That is true, but at least the user can work
around that by removing the onWindowExpiration annotation and modifying their
code. Currently there is no mechanism in the dataflow streaming backend to scan
and delivering an onDrain notification for all keys with user state, though
that would be possible to add. Any onDrain mechanism currently would have to
be powered by timers set for the end of time, which would have the same issues
as the cleanup timer.
----------------------------------------------------------------
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]
