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

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

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



##########
File path: runners/core-java/src/test/java/org/apache/beam/runners/core/SimpleDoFnRunnerTest.java
##########
@@ -386,6 +388,89 @@ public void testInfiniteSkew() {
             BoundedWindow.TIMESTAMP_MAX_VALUE));
   }
 
+  /**
+   * Demonstrates that attempting to set a timer with an output timestamp before the timestamp of
+   * the current element with zero {@link DoFn#getAllowedTimestampSkew() allowed timestamp skew}
+   * throws.
+   */
+  @Test
+  public void testTimerBackwardsInTimeNoSkew() {

Review comment:
       It would make more sense to make these tests `@ValidatesRunner` tests then use the deprecated `DoFnRunner`. This will allow us to get coverage across multiple implementations.

##########
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),

Review comment:
       Do we need to make a similar change in other locations like `FnApiDoFnRunner.java`?




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