lukecwik commented on a change in pull request #11607:
URL: https://github.com/apache/beam/pull/11607#discussion_r421824248
##########
File path:
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/splittabledofn/WatermarkEstimators.java
##########
@@ -37,14 +37,16 @@
private Instant lastReportedWatermark;
public Manual(Instant watermark) {
- this.watermark = checkNotNull(watermark, "watermark must not be null.");
- if (watermark.isBefore(GlobalWindow.TIMESTAMP_MIN_VALUE)
- || watermark.isAfter(GlobalWindow.TIMESTAMP_MAX_VALUE)) {
- throw new IllegalArgumentException(
- String.format(
- "Provided watermark %s must be within bounds [%s, %s].",
- watermark, GlobalWindow.TIMESTAMP_MIN_VALUE,
GlobalWindow.TIMESTAMP_MAX_VALUE));
+ checkNotNull(watermark, "watermark must not be null.");
+
+ // Making sure that the watermark is within bounds.
Review comment:
The bounds are wrong, they should be `BoundedWindow.TIMESTAMP_MIN_VALUE`
and `BoundedWindow.TIMESTAMP_MAX_VALUE`. This could have been the issue with
the timestamp being out of range. If that is the case then I would rather keep
the bounds as they were and make setWatermark more restrictive. We should also
update the javadoc for setWatermark/currentWatermark (on the interfaces) saying
that the value must always be within these bounds.
I think it makes more sense keep the restriction here and also make the
restriction within setWatermark as well so the user gets an error when setting
it.
----------------------------------------------------------------
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:
us...@infra.apache.org
