-
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
Fix: append all slice data when range for it #16327
Conversation
Signed-off-by: alingse <alingse@foxmail.com>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
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.
Looks good to me, thank you for the contribution. Where you able to find any other cases where we do this?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16327 +/- ##
==========================================
- Coverage 68.72% 68.72% -0.01%
==========================================
Files 1547 1547
Lines 198270 198270
==========================================
- Hits 136270 136258 -12
- Misses 62000 62012 +12 ☔ View full report in Codecov by Sentry. |
the linter result only show this. I think the others are correct. |
@@ -838,7 +838,7 @@ func keyspaceParamsToKeyspaces(ctx context.Context, wr *wrangler.Wrangler, param | |||
} | |||
if param[0] == '/' { | |||
// this is a topology-specific path | |||
result = append(result, params...) | |||
result = append(result, param) |
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 do you think that this was not intentional? It seems to be intentional to me, in that we are passing along all of the arguments.
Also, please note that vtctl
has been deprecated since v19 and is likely to be removed in v21 or v22.
In any event, this function is currently only called from here:
func commandRebuildKeyspaceGraph(ctx context.Context, wr *wrangler.Wrangler, subFlags *pflag.FlagSet, args []string) error {
cells := subFlags.String("cells", "", "Specifies a comma-separated list of cells to update")
allowPartial := subFlags.Bool("allow_partial", false, "Specifies whether a SNAPSHOT keyspace is allowed to serve with an incomplete set of shards. Ignored for all other types of keyspaces")
if err := subFlags.Parse(args); err != nil {
return err
}
if subFlags.NArg() == 0 {
return fmt.Errorf("the <keyspace> argument must be used to specify at least one keyspace when calling the RebuildKeyspaceGraph command")
}
var cellArray []string
if *cells != "" {
cellArray = strings.Split(*cells, ",")
}
keyspaces, err := keyspaceParamsToKeyspaces(ctx, wr, subFlags.Args())
So I believe the topology path portion of this function is dead code 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.
From the overall code perspective, I don't see any indication that this was intentional. If what you're saying is true, then any param in the params that starts with a '/', would cause the entire params to be appended to the result, even if one of the params is an empty string, or a wildcard matching path.
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.
Right, from looking at the code I think the idea was that the parameters to that function could be from the RebuildKeyspaceGraph command, in which case they are keyspaces. Or it could be from some other function, which doesn't seem to exist any longer, and in which case the parameters were topology paths.
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.
So I'd rather we remove that block entirely since it's dead code. But also, it's dead code in a deprecated client so I'm also fine leaving it as-is.
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.
Do you want to just remove that IF/ELSE block along with what was in the IF condition? We'd achieve the same goal here while also removing dead code. Thanks!
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 don't know the whole logic of the vitess, so I can't do the things that fix it by removing the dead code, you can removing the dead code and close this PR.
You'll need to rebase your PR on origin/main or merge in origin/main to fix the test failures you're seeing here. |
thank for notice, I have sync the commits from vitess:main |
Description
I am writing a linter called sundrylint to address some real-world bugs that I discovered during my work.
When we range over a slice and call the append function within the loop body, we are not appending all elements, but only the ones that we specifically iterate over during the range operation.
I read the code in here and I think it was just want to append that one
param
toresult
Related Issue(s)
Checklist
Deployment Notes