kennknowles commented on code in PR #32044:
URL: https://github.com/apache/beam/pull/32044#discussion_r1714010606
##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/FnApiDoFnRunner.java:
##########
@@ -327,6 +328,11 @@ static class Factory<InputT, RestrictionT, PositionT,
WatermarkEstimatorStateT,
* otherwise.
*/
private RestrictionTracker<RestrictionT, PositionT> currentTracker;
+ /**
+ * If non-null, set to true after currentTracker has had a tryClaim issued
on it. Used to ignore
Review Comment:
If it can be null, mark `@Nullable` and document the meaning of a null
value. (it looks to me like it is just "in between calls or during calls where
it is not used" but it is hard to ascertain design intent of this class TBH)
##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/FnApiDoFnRunner.java:
##########
@@ -1278,6 +1299,13 @@ private HandlesSplits.SplitResult
trySplitForWindowObservingTruncateRestriction(
if (currentWindow == null) {
return null;
}
+ // We are requesting a checkpoint but have not yet progressed on the
restriction, skip
+ // request.
+ if (fractionOfRemainder == 0
+ && currentTrackerClaimed != null
Review Comment:
Given it is not marked `volatile` those could be out of date, right? Based
on this and your use of `lazySet` that seems to align with your intent, but I
don't have a holistic understanding of this code.
--
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-unsubscr...@beam.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
