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 @@ export class ProgressPrinter {
}

public start() {
// If there is already a running setInterval, throw an error.
// This is because if this.setInterval is reassigned to another setInterval,
// the original setInterval remains and can no longer be cleared.
if (this.setInterval) {
throw new Error('ProgressPrinter is already running. Stop it first using the stop() method before starting it again.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
throw new Error('ProgressPrinter is already running. Stop it first using the stop() method before starting it again.');
throw new ToolkitError('ProgressPrinter is already running. Stop it first using the stop() method before starting it again.');

Copy link
Contributor

Choose a reason for hiding this comment

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

import { ToolkitError } from '../../toolkit/error';

^ needs to be added at the top of this file @sakurai-ryo. I don't have push access to your fork.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

this.setInterval = setInterval(() => {
if (!this.isPaused) {
this.print();
Expand Down
28 changes: 28 additions & 0 deletions packages/aws-cdk/test/api/garbage-collection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
BackgroundStackRefresh,
BackgroundStackRefreshProps,
} from '../../lib/api/garbage-collection/stack-refresh';
import { ProgressPrinter } from '../../lib/api/garbage-collection/progress-printer';
import {
BatchDeleteImageCommand,
BatchGetImageCommand,
Expand Down Expand Up @@ -1002,6 +1003,33 @@ describe('BackgroundStackRefresh', () => {
});
});

describe('ProgressPrinter', () => {
let progressPrinter: ProgressPrinter;
let setInterval: jest.SpyInstance;
let clearInterval: jest.SpyInstance;

beforeEach(() => {
jest.useFakeTimers();
setInterval = jest.spyOn(global, 'setInterval');
clearInterval = jest.spyOn(global, 'clearInterval');

progressPrinter = new ProgressPrinter(0, 1000);
});

afterEach(() => {
jest.clearAllTimers();
setInterval.mockRestore();
clearInterval.mockRestore();
});

test('throws if start is called twice without stop', () => {
progressPrinter.start();

expect(setInterval).toHaveBeenCalledTimes(1);
expect(() => progressPrinter.start()).toThrow('ProgressPrinter is already running. Stop it first using the stop() method before starting it again.');
});
});

function daysInThePast(days: number): Date {
const d = new Date();
d.setDate(d.getDate() - days);
Expand Down
Loading