[GitHub] [beam] laraschmidt commented on a change in pull request #15540: Allow for DoFn#getAllowedTimestampSkew() when checking the output timestamp

[GitBox <gi...@apache.org>]

laraschmidt commented on a change in pull request #15540:
URL: https://github.com/apache/beam/pull/15540#discussion_r713451202



##########
File path: runners/core-java/src/main/java/org/apache/beam/runners/core/SimpleDoFnRunner.java
##########
@@ -1190,12 +1190,12 @@ public Timer withOutputTimestamp(Instant outputTimestamp) {
      * </ul>
      */
     private void setAndVerifyOutputTimestamp() {
-
       if (outputTimestamp != null) {
         checkArgument(
-            !outputTimestamp.isBefore(elementInputTimestamp),
-            "output timestamp %s should be after input message timestamp or output timestamp of firing timers %s",
+            !outputTimestamp.plus(fn.getAllowedTimestampSkew()).isBefore(elementInputTimestamp),

Review comment:
       Seems like it's not handled by the instant. I copied what is done above which is to just special case the MAX_VALUE. Technically I think this could still overflow if it's almost the MAX_VALUE? But seems like it's pretty unlikely that it would be almost max value but not max value. I also added a test for the max value case.




-- 
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.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org