jaketf commented on a change in pull request #11596:
URL: https://github.com/apache/beam/pull/11596#discussion_r421824462
##########
File path:
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/splittabledofn/OffsetRangeTracker.java
##########
@@ -94,6 +94,9 @@ public void checkDone() throws IllegalStateException {
if (range.getFrom() == range.getTo()) {
return;
}
+ if (lastAttemptedOffset == null) {
+ throw new IllegalStateException("lastAttemptedOffset should not be
null");
+ }
checkState(
lastAttemptedOffset >= range.getTo() - 1,
"Last attempted offset was %s in range %s, claiming work in [%s, %s)
was not attempted",
Review comment:
Sure, I can move this to separate PR.
Kindly, disagree on suggestion as it will just cause a different NPE on
[L102](https://github.com/apache/beam/pull/11596/files/d2a094d5a133132d015fc7ed335e5b430a19f183#diff-b07e9d49c549836ef7e2d7006fa93f3cL102)
when you call `lastAttemptedOffset + 1` to try and print this error message.
I think the lastAttemptedOffset null check should be separate and throw a
more specific error message before we get to this stateCheck.
This failure mode is almost definitely mis-use of OffsetRangeTracker that
would even cause this, and would be difficult to say what work was or wasn't
attempted.
----------------------------------------------------------------
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]
