secondary: add SummarizeErrors variadic method #96
Conversation
|
|
HonoreDB
force-pushed
the
summarize_errors
branch
from
3057bb0
to
c224793
Compare
|
Hi sorry I did not see this earlier. |
|
Sure, the file that motivated this is https://github.com/cockroachdb/cockroach/blob/master/pkg/ccl/changefeedccl/sink_pubsub.go#L272 . I've linked one line in particular where I'm calling CombineErrors on the output of another function in the same file that also calls CombineErrors. It's a flow where errors can be encountered and persisted in different ways at different points by workers, and then most of the exposed methods need to fail informatively if there's been an error in any of those places. |
I was recently debugging a workflow with lots of async workers able to cancel each other, so that by the time an error bubbled up it was ambiguous where the longest stack would be and whether there were errors in two places because one wrapped the other or because of two separate root errors. I felt like what would be most convenient would be to smoosh everything together rather than try to be too smart about it. This PR implements SummarizeErrors, which is mainly the generalization of CombineErrors over n errors. If one argument is detectably already wrapping another, the result will ignore the latter. If two errors are distinct but both wrapping the same error, the result will preserve that information. Two errors are distinct if err1 != err2; there didn't look to be much benefit in bringing in things like .Is as we're really interested in identity, not equivalence.
HonoreDB
force-pushed
the
summarize_errors
branch
from
c224793
to
3e99fe9
Compare
|
Very non-urgent ping. |
There was a problem hiding this comment.
I had another look at this. Right now, this code it a little bit too ad-hoc in its treatment of secondaryError.
May I instead suggest replacing the secondaryError struct altogether, with something that supports a set of secondary errors instead of just one? I'm thinking specifically about this feature:
https://github.com/knz/shakespeare/blob/master/pkg/cmd/errors.go#L38
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @HonoreDB)
I was recently debugging a workflow with lots
of async workers able to cancel each other,
so that by the time an error bubbled up it was
ambiguous where the longest stack would be
and whether there were errors in two places
because one wrapped the other or because of two
separate root errors. I felt like what would be
most convenient would be to smoosh everything
together rather than try to be too smart about it.
This PR implements SummarizeErrors, which is mainly
the generalization of CombineErrors over n errors.
If one argument is detectably already wrapping another,
the result will ignore the latter. If two errors are
distinct but both wrapping the same error, the result
will preserve that information.
Two errors are distinct if err1 != err2; there didn't
look to be much benefit in bringing in things like .Is
as we're really interested in identity, not equivalence.
This is an unsolicited spike; feel free to bikeshed details or reject out of hand.
This change is