Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -251,9 +251,10 @@ export class GarbageCollector {

debug(`Parsing through ${numImages} images in batches`);

printer.start();

for await (const batch of this.readRepoInBatches(ecr, repo, batchSize, currentTime)) {
await backgroundStackRefresh.noOlderThan(600_000); // 10 mins
printer.start();

const { included: isolated, excluded: notIsolated } = partition(batch, asset => !asset.tags.some(t => activeAssets.contains(t)));

Expand Down Expand Up @@ -323,12 +324,13 @@ export class GarbageCollector {

debug(`Parsing through ${numObjects} objects in batches`);

printer.start();

// Process objects in batches of 1000
// This is the batch limit of s3.DeleteObject and we intend to optimize for the "worst case" scenario
// where gc is run for the first time on a long-standing bucket where ~100% of objects are isolated.
for await (const batch of this.readBucketInBatches(s3, bucket, batchSize, currentTime)) {
await backgroundStackRefresh.noOlderThan(600_000); // 10 mins
printer.start();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like the actual fix!


const { included: isolated, excluded: notIsolated } = partition(batch, asset => !activeAssets.contains(asset.fileName()));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@
}

public start() {
// If there is already a running setInterval, clear it first.
// This is because if this.setInterval is reassigned to another setInterval,
// the original setInterval remains and can no longer be cleared.
if (this.setInterval) {
clearInterval(this.setInterval);

Check warning on line 48 in packages/aws-cdk/lib/api/garbage-collection/progress-printer.ts

View check run for this annotation

Codecov / codecov/patch

packages/aws-cdk/lib/api/garbage-collection/progress-printer.ts#L48

Added line #L48 was not covered by tests
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find, but I'd rather see this thrown an exception saying that stop() should have been called first.

The problem is not that start() doesn't do what it should, the problem is that the consumer is calling this class incorrectly, but it's not obvious that it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that makes sense.
f7726d9

this.setInterval = setInterval(() => {
if (!this.isPaused) {
this.print();
Expand Down
Loading