je-ik commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-846842691
@boyuanzz @reuvenlax @kennknowles I'd like to reach a consensus here. We
have four options:
a) leave thing as they are
b) relax the check, so that it checks only that the output timestamp is not
after window GC time
c) modify the check so that it checks the output timestamp is not after
window.maxTimestamp
d) remove the check completely
I think a strong arguments were made that option a) is wrong. Option c)
seems to be a breaking change to me - existing pipelines, that use
withOutputTimestamp might break, because there is nothing stopping them (now)
to use output timestamp that is higher than window.maxTimestamp. Therefore, I
think we should rule option c) out as well.
This leaves options b) and d) - option b) is in this PR, option d) is
simplistic modification. :-)
From my point of view, because it is legitimate to use `withOutputTimestamp`
anytime an output is generated and there is nothing stopping the user to output
the element arbitrarily far in the future, we should use the same logic and
allow the same for timer output timestamps.
So, to conclude - I'm fine with both b) and d), while d) seems to be the
most clean solution.
--
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]
