-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[wrangler|topotools] Migrate UpdateShardRecords
, RefreshTabletsByShard
, and {Get,Save}RoutingRules
#7965
[wrangler|topotools] Migrate UpdateShardRecords
, RefreshTabletsByShard
, and {Get,Save}RoutingRules
#7965
Conversation
…}RoutingRules` I am moving these to `package topotools` to reuse this code in both `package grpcvtctldserver` (which wrangler imports) and `package wrangler`. Signed-off-by: Andrew Mason <amason@slack-corp.com>
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.
This looks good. Left a small comment. Let me know what you think.
go/vt/topotools/keyspace.go
Outdated
// For 'to' shards, refresh to make them serve. The 'from' shards will | ||
// be refreshed after traffic has migrated. | ||
if !isFrom { | ||
_ = RefreshTabletsByShard(ctx, ts, tmc, si, cells, logger) |
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.
I like this pattern of being explicit that we are ignoring the error. However, is not a pattern that I've seen in the codebase.
I wonder if start adding here will create confusion.
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.
My opinion is being that explicit about ignoring the error is a better (or at least more clear) pattern, and we've got to start somewhere! 😂 I'll do a quick temp check to see what some other folks think about this
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.
That sounds like a plan. If we can get thumbs-up that we would like to make this a standard pattern moving forward, let's start here!
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.
Consensus seemed to be:
- yes, explicitly acknowledging that we're ignoring errors is better than implicitly doing so
- we've had lots of trouble debugging tests specifically due to errors being discarded
- we should log the ignored error in this specific case, and whether to log or not is probably a case-by-case basis
- i'm also going to work on adding
errcheck
to the linter so we can start to enforce this as a pattern going forward
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.
Great, thanks for following up on this. All makes sense.
Signed-off-by: Andrew Mason <amason@slack-corp.com>
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.
LGTM
…funcs_to_topotools [wrangler|topotools] Migrate `UpdateShardRecords`, `RefreshTabletsByShard`, and `{Get,Save}RoutingRules`
…funcs_to_topotools [wrangler|topotools] Migrate `UpdateShardRecords`, `RefreshTabletsByShard`, and `{Get,Save}RoutingRules` Signed-off-by: Vitaliy Mogilevskiy <vmogilevskiy@slack-corp.com>
Description
I am moving these to
package topotools
to reuse this code in bothpackage grpcvtctldserver
(which wrangler imports) andpackage wrangler
.Signed-off-by: Andrew Mason amason@slack-corp.com
Related Issue(s)
Checklist
Deployment Notes