scwhittle commented on code in PR #36750:
URL: https://github.com/apache/beam/pull/36750#discussion_r2509404400
##########
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:
What about the first concern that caller may observe stale progress in cases
it itself is calling in order (via being on the same thread or some other
happens-before relationship)?
For example, if the split processing thread calls:
- trySplit
- getProgress
then the getProgress response might be stale from before the split. This
could possibly confuse the runner. I'm not sure if this is the case or not but
I think we'd have to follow the code to make sure this doesn't cause some
subtle corruption.
@kennknowles might know
The reason I was suggesting fixing Read itself is that other
RestrictionTrackers used by other SDFs don't do expensive work in
RestrictionTracker.tryClaim implementation. It is generally just recording the
offset etc. Requiring that getProgress is responsive doesn't seem to onerous a
requirement for a RestrictionTracker if we document it and then we don't have
to worry about these possibly stale responses mixing things up.
--
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]
