scwhittle commented on code in PR #36750:
URL: https://github.com/apache/beam/pull/36750#discussion_r2503965467
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/fn/splittabledofn/RestrictionTrackers.java:
##########
@@ -101,8 +141,16 @@ protected RestrictionTrackerObserverWithProgress(
}
@Override
- public synchronized Progress getProgress() {
- return ((HasProgress) delegate).getProgress();
+ public Progress getProgress() {
Review Comment:
If the caller has ordering (like being on the same thread) that guarantees
that this is called after a trySplit or tryClaim succeeds, it may be
unexpected that this returns a stale progress and lead to weird failures.
Another approach to fixing this would be to make
RestrictionTracker.getProgress() quick. In general restriction trackers are
lightweight, the ones in Read.java are odd in that they are abusing the
restriction tracker tryClaim return payloads. I think that we could instead
change them to not do so beneath RestrictionTracker.tryClaim as another way to
fix this.
hacky example, needs more thought on trySplit:
https://github.com/apache/beam/pull/36758
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/fn/splittabledofn/RestrictionTrackers.java:
##########
@@ -101,8 +141,16 @@ protected RestrictionTrackerObserverWithProgress(
}
@Override
- public synchronized Progress getProgress() {
- return ((HasProgress) delegate).getProgress();
+ public Progress getProgress() {
+ pendingProgress = true;
+ if (lock.tryLock()) {
Review Comment:
what about using tryLock with timeout to wait for up to X seconds? That
seems responsive enough and will help make sure contention doesn't prevent
observing the most recent progress
we could also consider logging that we're going to return a stale progress
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
