[Feature] [Platform] Improve Registry Performance #1934
Conversation
ajanikow
force-pushed
the
feature/platform/improve_registry_performance
branch
from
38b9fa2 to
d9953f2
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces improvements to registry client performance by adding configurable request concurrency and a new registry list flag.
- Default
ReqConcurrentis set to 8 for all registry hosts. - A new
registry.docker.endpointflag (flagRegistryList) is added and registered in import/export commands. - Documentation, CRD schemas, and changelog are updated to reflect selector description fixes and the new feature.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/platform/regclient.go | Set default ReqConcurrent and apply flagRegistryList values |
| pkg/platform/flags.go | Define new flagRegistryList for boosted registries |
| pkg/platform/package_import.go | Register flagRegistryList in import command |
| pkg/platform/package_export.go | Register flagRegistryList in export command |
| pkg/crd/crds/backups-backuppolicy.schema.generated.yaml | Update selector description to "ArangoDeployment" |
| pkg/apis/backup/v1/backup_policy_spec.go | Fix selector comment, bump copyright year |
| docs/api/ArangoBackupPolicy.V1.md | Update selector description to "ArangoDeployment" |
| CHANGELOG.md | Add entry for "Improve Registry Performance" |
Comments suppressed due to low confidence (2)
pkg/platform/flags.go:139
- [nitpick] The flag name "registry.docker.endpoint" is misleading given it represents a list of registries. Consider renaming it to something like "registry.docker.list" or updating the description to match the name.
Name: "registry.docker.endpoint",
pkg/platform/regclient.go:67
- No tests cover the new
flagRegistryListparsing or the defaultReqConcurrentbehavior. Adding unit tests for these paths would help prevent regressions.
regs, err := flagRegistryList.Get(cmd)
| @@ -35,6 +35,10 @@ func getRegClient(cmd *cobra.Command) (*regclient.RegClient, error) { | |||
|
|
|||
| slog.SetLogLoggerLevel(slog.LevelDebug) | |||
|
|
|||
| flags = append(flags, regclient.WithConfigHostDefault(config.Host{ | |||
There was a problem hiding this comment.
The literal value ReqConcurrent: 8 appears multiple times. Extract this into a named constant (e.g., defaultReqConcurrent) to avoid magic numbers and improve maintainability.
Copilot uses AI. Check for mistakes.
| v.Hostname = el | ||
| } | ||
|
|
||
| v.ReqConcurrent = 8 |
There was a problem hiding this comment.
The concurrency setting is applied twice in separate loops. Consider consolidating this logic to ensure DRY code and reduce potential for divergence in future updates.
| v.ReqConcurrent = 8 | |
| v = setReqConcurrent(v) |
Copilot uses AI. Check for mistakes.
No description provided.