Skip to content

Commit

Permalink
Distinguish fatal vs non-fatal retryable errors
Browse files Browse the repository at this point in the history
closes #1327
  • Loading branch information
mkrufky committed Feb 14, 2020
1 parent a479141 commit bb32d4c
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 8 deletions.
2 changes: 1 addition & 1 deletion verification/epic.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ var VerifierPath string
var ErrMissingSource = errors.New("MissingSource")
var ErrVerifierStatus = errors.New("VerifierStatus")
var ErrVideoUnavailable = errors.New("VideoUnavailable")
var ErrAudioMismatch = Retryable{errors.New("AudioMismatch")}
var ErrAudioMismatch = Fatal{Retryable{errors.New("AudioMismatch")}}
var ErrTampered = Retryable{errors.New("Tampered")}

type epicResolution struct {
Expand Down
21 changes: 14 additions & 7 deletions verification/verify.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ type Retryable struct {
error
}

var ErrPixelMismatch = Retryable{errors.New("PixelMismatch")}
type Fatal struct {
Retryable
}

var ErrPixelMismatch = Fatal{Retryable{errors.New("PixelMismatch")}}
var ErrPixelsAbsent = errors.New("PixelsAbsent")

type Params struct {
Expand Down Expand Up @@ -85,7 +89,6 @@ func (a byResScore) Less(i, j int) bool { return a[i].res.Score < a[j].res.Score
type SegmentVerifier struct {
policy *Policy
results []SegmentVerifierResults
count int
}

func NewSegmentVerifier(p *Policy) *SegmentVerifier {
Expand Down Expand Up @@ -127,18 +130,17 @@ func (sv *SegmentVerifier) Verify(params *Params) (*Params, error) {
// Verification passed successfully, so use this set of params
return params, nil
}
sv.count++

// Append retryable errors to results
// Append non-fatal retryable errors to results
// The caller should terminate processing for non-retryable errors
if IsRetryable(err) {
if !IsFatal(err) && IsRetryable(err) {
r := SegmentVerifierResults{params: params, res: res}
sv.results = append(sv.results, r)
}

// Check for max retries
// If max hit, return best params so far
if sv.count > sv.policy.Retries {
if len(sv.results) > sv.policy.Retries {
if len(sv.results) <= 0 {
return nil, err
}
Expand All @@ -149,7 +151,12 @@ func (sv *SegmentVerifier) Verify(params *Params) (*Params, error) {
return nil, err
}

func IsFatal(err error) bool {
_, fatal := err.(Fatal)
return fatal
}

func IsRetryable(err error) bool {
_, retryable := err.(Retryable)
return retryable
return retryable || IsFatal(err)
}
33 changes: 33 additions & 0 deletions verification/verify_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,39 @@ import (
"github.com/livepeer/go-livepeer/net"
)

func TestFatalRetryable(t *testing.T) {

This comment has been minimized.

Copy link
@j0sh

j0sh Feb 14, 2020

Contributor

Could we also add a test that ensures the following condition is no longer an issue:

  1. O returns a legitimate result with a high score
  2. The same results have an incorrect pixel count
  3. Verifier detects both the high score and the pixel mismatch
  4. Because pixel mismatch errors are marked as retryable, the O with the high score may have its results selected even if its reported pixel count is incorrect.

Originally described in #1096 (comment)

errNonRetryable := errors.New("NonRetryable")
assert.False(t, IsRetryable(errNonRetryable))
assert.False(t, IsFatal(errNonRetryable))

errRetryable := Retryable{errors.New("Retryable")}
assert.True(t, IsRetryable(errRetryable))
assert.False(t, IsFatal(errRetryable))

errFatalRetryable := Fatal{Retryable{errors.New("FatalRetryable")}}
assert.True(t, IsRetryable(errFatalRetryable))
assert.True(t, IsFatal(errFatalRetryable))

errNonFatalRetryable := Retryable{errors.New("NonFatalRetryable")}
assert.True(t, IsRetryable(errNonFatalRetryable))
assert.False(t, IsFatal(errNonFatalRetryable))

// check that these Retryable errors are classified
// correctly as Fatal / Non-fatal:

// check ErrAudioMismatch
assert.True(t, IsRetryable(ErrAudioMismatch))
assert.True(t, IsFatal(ErrAudioMismatch))

// check ErrPixelMismatch
assert.True(t, IsRetryable(ErrPixelMismatch))
assert.True(t, IsFatal(ErrPixelMismatch))

// check ErrTampered
assert.True(t, IsRetryable(ErrTampered))
assert.False(t, IsFatal(ErrTampered))
}

type stubVerifier struct {
results *Results
err error
Expand Down

0 comments on commit bb32d4c

Please sign in to comment.