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
|
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.
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.
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.
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.
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.
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.
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
paramtoresultRelated Issue(s)
Checklist
Deployment Notes