Conversation
Wwwsylvia
requested review from
adewaleo,
sajayantony,
shizhMSFT and
yugangw-msft
as code owners
cmd/worker/purger.go
Outdated
| // NewPurger creates a new Purger. | ||
| func NewPurger(batchSize int, acrClient api.AcrCLIClientInterface) *Purger { | ||
| procNum := int64(runtime.GOMAXPROCS(0)) | ||
| worker := newPurgeWorker(acrClient) |
There was a problem hiding this comment.
newPurgeWorker [](start = 11, length = 14)
It is not a worker. It is a job #Resolved
cmd/acr/purge.go
Outdated
| if wErr.Error != nil { | ||
| return -1, wErr.Error | ||
| // jobs to be finished before continuing. | ||
| p.Wait(ctx) |
There was a problem hiding this comment.
There will be a dead lock if there are more tags than the batch size. #Resolved
| workerPool: workerPool, | ||
| acrClient: acrClient, | ||
| wg: sync.WaitGroup{}, | ||
| } |
There was a problem hiding this comment.
nit: no need to initialize #Resolved
cmd/worker/purger.go
Outdated
| func (p *Purger) StartPurgeManifest(ctx context.Context, loginURL string, repoName string, digest string) { | ||
| job := newPurgeManifestJob(loginURL, repoName, digest) | ||
|
|
||
| p.workerPool.start(ctx, job, p.acrClient, &p.wg) |
There was a problem hiding this comment.
&p.wg [](start = 43, length = 5)
Please keep the wait group inside purger and not pass to the worker pool #Resolved
cmd/acr/purge.go
Outdated
| cmd.Flags().IntVar(&purgeParams.keep, "keep", 0, "Number of latest to-be-deleted tags to keep, use this when you want to keep at least x number of latest tags that could be deleted meeting all other filter criteria") | ||
| cmd.Flags().StringArrayVarP(&purgeParams.filters, "filter", "f", nil, "Specify the repository and a regular expression filter for the tag name, if a tag matches the filter and is older than the duration specified in ago it will be deleted") | ||
| cmd.Flags().StringArrayVarP(&purgeParams.configs, "config", "c", nil, "Authentication config paths (e.g. C://Users/docker/config.json)") | ||
| cmd.Flags().IntVarP(&purgeParams.taskNumber, "task-number", "t", 0, taskNumberDescription) |
There was a problem hiding this comment.
0 [](start = 66, length = 1)
default value should be set here. #Resolved
| err = purger.FlushErrChan() | ||
| if err != nil { | ||
| return -1, err | ||
| } |
There was a problem hiding this comment.
We probably need to re-design this part. The objective is that we should return error if there is an error. #Resolved
|
@shizhMSFT Please help to review, Thanks! |
cmd/worker/pool.go
Outdated
|
|
||
| // pool manages a limited number of workers that process purgeJob. | ||
| type pool struct { | ||
| size int |
There was a problem hiding this comment.
size int [](start = 1, length = 8)
It seems redundant. #Resolved
2 participants
Refactor the worker package to limit amount of concurrent DELETE requests to ACR server