Change source reporter interfaces to not return errors #3900
Conversation
| return reporter.UnitErr(ctx, err) | ||
| reporter.UnitErr(ctx, err) | ||
| return nil |
There was a problem hiding this comment.
For the cases where we were returning the reporter error, I changed it to return nil. This would be a behavior change for context cancellations, so maybe we should return ctx.Err() instead. Thoughts?
There was a problem hiding this comment.
I think that returning ctx.Err() instead is a good idea!
mcastorina
force-pushed
the
rm-reporter-errs
branch
from
81aac74 to
471744c
Compare
| func (c ChanReporter) ChunkOk(ctx context.Context, chunk Chunk) error { | ||
| return common.CancellableWrite(ctx, c.Ch, &chunk) | ||
| func (c ChanReporter) ChunkOk(ctx context.Context, chunk Chunk) { | ||
| _ = common.CancellableWrite(ctx, c.Ch, &chunk) |
There was a problem hiding this comment.
Can you leave a comment explaining what kind of errors can get swallowed here and why that's ok to do?
That's correct. Additionally, it removes confirmation of whether the item was successfully handled. That is the main tradeoff of this change. The argument is that sources shouldn't need to care whether the item was handled or not. They do need to be context aware, though. |
Yep, so then should this PR re-add the (implicit) context cancellation checks that it's removed? |
Description:
The error was meant to allow the reporter to end scanning, but in practice would only be useful for context cancellations, which sources should be handling anyway. Removing the error simplifies the usage of the interface.
Checklist:
make test-community)?make lintthis requires golangci-lint)?