treat sync1.1 errors as warnings #989
Conversation
There was a problem hiding this comment.
overall looks good, and a couple good little cleanups along the way.
I flagged a couple places we probably don't need warn-versus-error: #sync events aren't emitted by pre-sync1.1 software at all, and DID syntax in #commit events can always be enforced strictly (not a sync1.1 change)
|
|
||
| did, err := syntax.ParseDID(msg.Did) | ||
| if err != nil { | ||
| syncVerifyErrors.WithLabelValues(hostname, "did").Inc() | ||
| return nil, err | ||
| if !val.Sync11ErrorsAreWarnings { |
There was a problem hiding this comment.
I think all these #sync event checks could be errors-not-warnings. The whole event is new in sync v1.1, so there shouldn't be any problem with old implementations outputting events which don't validate. It is only the #commit events we need to handle with warning-not-error.
There was a problem hiding this comment.
this is where I thought the discussion went yesterday, that we have to allow mediocre implementation of sync1.1 for a while
brianolson
force-pushed
the
bolson/sync11-error-as-warn
branch
from
d604306 to
e5f3163
Compare
cmd/relay/bgs/validator.go
Outdated
| commit, err := atrepo.LoadCARCommit(ctx, bytes.NewReader([]byte(msg.Blocks))) | ||
| if err != nil { | ||
| commitVerifyErrors.WithLabelValues(hostname, "car").Inc() | ||
| return nil, err | ||
| if !val.Sync11ErrorsAreWarnings { | ||
| return nil, err | ||
| } else { | ||
| logger.Warn("invalid car", "err", err) | ||
| } | ||
| } | ||
|
|
||
| if commit.Rev != rev.String() { |
There was a problem hiding this comment.
If there was an error in loading the CAR, I would expect commit to be nil. If that's the case, falling through on the "invalid car" warn would cause a nil pointer exception here checking the rev.
There was a problem hiding this comment.
fixed (and in commit handler)
cmd/relay/bgs/validator.go
Outdated
| commit, err := atrepo.LoadCARCommit(ctx, bytes.NewReader([]byte(msg.Blocks))) | ||
| if err != nil { | ||
| commitVerifyErrors.WithLabelValues(hostname, "car").Inc() | ||
| if !val.Sync11ErrorsAreWarnings { | ||
| if !val.Sync11ErrorsAreWarnings || commit == nil { |
There was a problem hiding this comment.
I would expect that if err != nil that the commit is going to be nil. Is that not the case? I think that means we always have to error if we fail to read the commit from the CAR, there's no case where we'll end-up warning here.
There was a problem hiding this comment.
ok, yeah, content and err are mutually exclusive, cleaned up
No description provided.