support SCATTER_ERRORS_AS_WARNINGS query directive#4228
support SCATTER_ERRORS_AS_WARNINGS query directive#4228sougou merged 10 commits intovitessio:masterfrom
Conversation
|
@sougou I'd love your thoughts on the syntax proposal here. |
demmer
force-pushed
the
partial-scatter
branch
2 times, most recently
from
e2318ca to
ed19e00
Compare
demmer
force-pushed
the
partial-scatter
branch
from
ed19e00 to
49df52a
Compare
In order to enable callers to remember errors positionally for a given input array, add new RecordErrorAt and GetErrors interfaces. This feature will be used to store the errors for each shard in a parallel scatter query operation. Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
When running a multi-shard query, instead of returning a single aggregated array if any single shard fails, return an array of errors, one for each shard. This adds the necessary infrastructure for the executor to be able to return partial success if some shard queries fail but others succeed. Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Add a ShardPartial option to the route plan. If true, when there is at least one successful shard query, return the results that were obtained instead of an error. Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
demmer
force-pushed
the
partial-scatter
branch
2 times, most recently
from
ae1ea1b to
bfdc161
Compare
Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Given the updated design whereby vtgate will return any shard error as a warning, change the comment directive to more intuitively express what the behavior is. Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
demmer
force-pushed
the
partial-scatter
branch
from
bfdc161 to
c8a7bc5
Compare
demmer
changed the title
sougou
left a comment
sougou
left a comment
There was a problem hiding this comment.
The overall approach is good. But I think the error handling changes are not needed. Let me know if you had something else in mind.
go/vt/vtgate/scatter_conn.go
Outdated
| }) | ||
| return qr, err | ||
|
|
||
| // If there were any errors, return the |
go/vt/concurrency/error_recorder.go
Outdated
| // given capacity if required, so any future calls to | ||
| // RecordError will append to the array _after_ any | ||
| // nil entries created by growing the representation. | ||
| func (aer *AllErrorRecorder) GetErrors(n int) []error { |
There was a problem hiding this comment.
Do we need this? I think having multiGoTransaction return aer.AggrError(vterrors.Aggregate) will do the same thing. Then we won't have to change ExecuteMultiShard at all.
Along the same lines, RecordErrorAt is currently not useful because all code paths end up using vterrors.Aggregate which does its own sorting. So, if you want to retain the original order, you'll need to add an option to vterrors.Aggregate.
There was a problem hiding this comment.
You’re right. I needed to retain the position in the original design in which the executor would need to know which shard had an error.
Now that we just append all the errors to the warning list, we can remove this and go back to the simpler collector.
There was a problem hiding this comment.
Actually just to be clear — we still need the list of errors to come out of the engine but not necessarily with the positional correspondence with the shards array.
It’s not exactly evident why in this pr but if you look at the one which puts it all together it makes more sense.
There was a problem hiding this comment.
I see. You need them for the upcoming RecordWarnings. Sounds good. So, we only need to get rid of RecordErrorAt.
There was a problem hiding this comment.
@sougou I just pushed the change to remove RecordErrorAt.
Since the design for partial scatter evolved to no longer require errors be associated with their position in the shards array, remove the now unnecessary support in the AllErrorRecorder. This reverts commit 47e09f6. Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
demmer
force-pushed
the
partial-scatter
branch
from
48aa179 to
9ad6a06
Compare
2 participants
Description
Adds the
SCATTER_ERRORS_AS_WARNINGSquery directive. (Part 1 of #4279)Details
Specific changes include: