Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
schemadiff: validate views' referenced columns #12147
schemadiff: validate views' referenced columns #12147
Changes from 11 commits
c4c213b
3dac907
e88d426
61de6b0
389872c
56c8421
11d6379
f4fd673
a4ae98d
ef16bdf
54fb73a
2d3e70b
b69ede6
bc34369
bc8799d
dd1bc82
7f6fd13
4939d01
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the Vitess concurrency package here along with
vterrors.Aggregate
? You can see that used throughout the code base if you look for.AggrError(vterrors.Aggregate)
. Between theconcurrency
andvterrors
packages we should have whatever related functionality you need here for error handling. For example:vitess/go/vt/wrangler/traffic_switcher.go
Lines 146 to 161 in 390ddad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it does use locking which is unnecessary here. Dunno if we care about the overhead of that here? That logic seems designed for concurrent error gathering which isn't what we're doing here.
Using
multierr
seems simpler here and it's already an indirect dependency? Not a really strong opinion though, we can also use this but it seems a bit off for what it was designed for.I think this logic here is temporary anyway, since once golang/go#53435 is available with Go 1.20 we probably want to switch to that anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @dbussink
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh, race condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How strongly do people feel about this? Looking at the two implementation
multierr
does seem to be more fitting to our purpose, but I don't feel strongly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you only need multiple errors you can use
vterrors.Aggregate()
, no need for the concurrency piece (as noted, geared toward N goroutines) like we do here e.g.:vitess/go/vt/vttablet/tabletmanager/vreplication/vcopier.go
Lines 597 to 621 in 3f55e40
I don't feel strongly about it though. Up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI -
sqlparser.Walk
will only return the error you return from inside the visitor function, and since you don't return any errors from that, no errors will ever make it out.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@systay ah, great! Thank you. I'll keep the check as it is, for safety, but good to know!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@systay Right, but the errors are gathered here in
errs
? And those are at the end returned if the walker itself doesn't error (which is not expected).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbussink yeah, that was my point. Not really necessary to catch the returned error from
sqlparser.Walk
since that is not how we are dealing with the errors. When I know it will never return an error, I usually write.OTOH - it's probably good defensive programming to do as @shlomi-noach is doing here and catching and checking the error anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@systay Right, guess as someone now knowing the details too much about how
Walk
is implemented, I wouldn't know that it can't return an error and would still write it defensively 😄.