Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve ScatterErrorsAsWarnings functionality #8139

Merged
merged 17 commits into from
May 20, 2021
Merged

Improve ScatterErrorsAsWarnings functionality #8139

merged 17 commits into from
May 20, 2021

Conversation

harshit-gangal
Copy link
Member

@harshit-gangal harshit-gangal commented May 17, 2021 •
edited by systay

Description

This PR improves ScatterErrorsAsWarnings by doing three things:

  • Make sure that it works in both OLAP and OLTP mode. It was previously not working in OLAP mode
  • When all shards fail, the query will still fail. ScatterErrorsAsWarnings should only kick in when a single or a few shards fail.
  • Adds better end to end tests to assert that everything works as expected
  • Support SHOW WARNINGS in OLAP mode

Note: this is a breaking change. Queries that failed on all shards were not returning an error earlier, but after this change, scatter errors are turned into warnings only if at least one shard was successful.

Checklist

  • Tests were added or are not required
  • Documentation was added or is not required

harshit-gangal and others added 8 commits May 17, 2021 13:51
@harshit-gangal
added e2e and unit test for scatter_errors_as_warnings
a84facf 
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
@systay
rewrote test
d7d79bd 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@harshit-gangal
minimize the test setup
312d51d 
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
@harshit-gangal
add test to CI
5c24a3e 
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
@harshit-gangal
added test expectations
f4f407d 
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
@harshit-gangal
handle merge sort in streaming to log errors as warns if set
3475620 
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
@systay
codegen
61265cf 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
ScatterErrorsAsWarnings should not kick in when all shards fail
9eae5b6 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay systay added Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature) labels May 18, 2021
@systay systay marked this pull request as ready for review May 18, 2021 10:15
@systay systay self-requested a review as a code owner May 18, 2021 10:15
@systay systay changed the title Errs as warns Improve ScatterErrorsAsWarnings functionality May 18, 2021
@systay systay requested a review from deepthi May 18, 2021 10:16
harshit-gangal and others added 3 commits May 18, 2021 18:32
@harshit-gangal
plan showwarnings
8bcd0ab 
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
@systay @harshit-gangal
clean up code a little
3ffa14c 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
filter out empty errors
2f2e59d 
Signed-off-by: Andres Taylor <andres@planetscale.com>
harshit-gangal and others added 3 commits May 19, 2021 11:29
@harshit-gangal
fix error checking
6447c84 
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
@systay
codegen
1bade57 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@GuptaManan100
fixed an end to end test
6fe8c55 
Signed-off-by: GuptaManan100 <manan@planetscale.com>
@harshit-gangal
Copy link
Member Author

Marked as breaking change. As earlier all the errors coming from executing the plan will be consumed as error, now at least 1 shard should return results then warnings are logged otherwise throws an error.

harshit-gangal
harshit-gangal commented May 19, 2021
View reviewed changes
go/test/endtoend/mysqlserver/mysql_server_test.go Outdated Show resolved Hide resolved
go/test/endtoend/mysqlserver/mysql_server_test.go Outdated Show resolved Hide resolved
go/test/endtoend/mysqlserver/mysql_server_test.go Outdated Show resolved Hide resolved
go/test/endtoend/vtgate/main_test.go Outdated Show resolved Hide resolved
go/vt/vtgate/engine/route.go Outdated Show resolved Hide resolved
go/vt/vtgate/engine/session_primitive.go Outdated Show resolved Hide resolved
go/vt/vtgate/executor_select_test.go Outdated Show resolved Hide resolved
go/vt/vtgate/executor_select_test.go Show resolved Hide resolved
@systay
address review comments
2cd845c 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
make test less fragile
721c070 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
ask for fields if we depend on them
7c85ced 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@harshit-gangal harshit-gangal merged commit d51a7ee into vitessio:master May 20, 2021
@harshit-gangal harshit-gangal deleted the errs-as-warns branch May 20, 2021 14:22
@deepthi deepthi mentioned this pull request Feb 25, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@systay systay systay approved these changes

@deepthi deepthi Awaiting requested review from deepthi

@GuptaManan100 GuptaManan100 Awaiting requested review from GuptaManan100

@rohit-nayak-ps rohit-nayak-ps Awaiting requested review from rohit-nayak-ps

Assignees
No one assigned
Labels
Breaking change Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@harshit-gangal @systay @GuptaManan100