[ 
https://issues.apache.org/jira/browse/BEAM-14536?focusedWorklogId=776873&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-776873
 ]

ASF GitHub Bot logged work on BEAM-14536:
-----------------------------------------

                Author: ASF GitHub Bot
            Created on: 01/Jun/22 13:46
            Start Date: 01/Jun/22 13:46
    Worklog Time Spent: 10m 
      Work Description: jrmccluskey commented on code in PR #17782:
URL: https://github.com/apache/beam/pull/17782#discussion_r886825913


##########
sdks/go/pkg/beam/io/rtrackers/offsetrange/offsetrange.go:
##########
@@ -208,9 +210,11 @@ func (tracker *Tracker) GetProgress() (done, remaining 
float64) {
        return
 }
 
-// IsDone returns true if the most recent claimed element is past the end of 
the restriction.
+// IsDone returns true if the most recent claimed element is past the end of 
the restriction
+// or if the restriction represents no work to be done (aka the start of the 
restriction is
+// greater than or equal to the end).
 func (tracker *Tracker) IsDone() bool {
-       return tracker.err == nil && tracker.claimed >= tracker.rest.End
+       return tracker.err == nil && (tracker.claimed >= tracker.rest.End || 
tracker.rest.Start >= tracker.rest.End)

Review Comment:
   Wouldn't the "done" case with claimed be `tracker.claimed >= 
(tracker.rest.End - 1)` since the range is [start, end)? 



##########
sdks/go/pkg/beam/core/runtime/exec/sdf.go:
##########
@@ -678,10 +678,6 @@ func (n *ProcessSizedElementsAndRestrictions) Checkpoint() 
([]*FullValue, error)
                return nil, addContext(err)
        }
 
-       if !n.rt.IsDone() {
-               return nil, addContext(errors.Errorf("Primary restriction %#v 
is not done. Check that the RTracker's TrySplit() at fraction 0.0 returns a 
completed primary restriction", n.rt))
-       }

Review Comment:
   I don't love removing this check since I think it has value in preventing a 
misconfigured RTracker from losing data. I also think that new behavior works 
with this if the `IsDone()` function for the offsetrange tracker is tweaked 
like I suggested 





Issue Time Tracking
-------------------

    Worklog Id:     (was: 776873)
    Time Spent: 2.5h  (was: 2h 20m)

> Offsetrange tracker panics when splitting at 0.0 without claiming work
> ----------------------------------------------------------------------
>
>                 Key: BEAM-14536
>                 URL: https://issues.apache.org/jira/browse/BEAM-14536
>             Project: Beam
>          Issue Type: Bug
>          Components: sdk-go
>            Reporter: Danny McCormick
>            Assignee: Danny McCormick
>            Priority: P2
>          Time Spent: 2.5h
>  Remaining Estimate: 0h
>
> Right now, if you try to call TrySplit on an offsetrange restriction with a 
> fraction of 0.0 and without first claiming work, it sets the primary 
> restriction to \{Start, Start-1}. This causes newSplitResult to panic - 
> https://github.com/apache/beam/blob/ff39fcb5229b15140e41a61bd09f7d590730e93a/sdks/go/pkg/beam/core/runtime/exec/sdf.go#L859



--
This message was sent by Atlassian Jira
(v8.20.7#820007)

Reply via email to