Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[reaper] re-write lambda #4135

Closed
wants to merge 7 commits into from
Closed

[reaper] re-write lambda #4135

wants to merge 7 commits into from

Conversation

twrichards
Copy link
Contributor

@twrichards twrichards commented Sep 4, 2023

Re-write the existing lambda, primarily to coordinate (and log) calls to the endpoints added in #4134. It's designed to run on a schedule (currently every 15mins, regardless of stage).

Each invocation it attempts to...

...logging the above and storing a permanent record to dedicated (& stage specific) S3 bucket.

* batch size is different per stage (proportional to env's ingenstion rate) but is 1000 for PROD (since this is the maximum batch for the bulk delete APIs etc. used under the hood).

@twrichards twrichards changed the base branch from reaper/endpoints to empty-cdk September 4, 2023 14:33
@twrichards twrichards force-pushed the reaper/lambda-rewrite branch from 5182e16 to 5651b94 Compare September 4, 2023 14:34
@twrichards twrichards force-pushed the reaper/lambda-rewrite branch 10 times, most recently from 2b398c9 to 8bc12e4 Compare September 4, 2023 15:52
@twrichards twrichards force-pushed the reaper/lambda-rewrite branch 6 times, most recently from c9f2de4 to 8154301 Compare September 5, 2023 08:22
@twrichards twrichards force-pushed the reaper/lambda-rewrite branch 2 times, most recently from 8bdad62 to 62aebfa Compare September 5, 2023 09:35
@twrichards twrichards force-pushed the reaper/lambda-rewrite branch from 62aebfa to ef84a18 Compare September 5, 2023 09:37
@twrichards twrichards marked this pull request as ready for review September 5, 2023 09:48
@twrichards twrichards requested a review from a team as a code owner September 5, 2023 09:48
# run: |
# npm install
# npm run riffraff-artefact
- name: Reaper
Copy link
Member

Choose a reason for hiding this comment

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

👏❤️👏❤️👏❤️

export const AWS_REGION = 'eu-west-1';
export const STACK = 'media-service';
export const REAPER_APP_NAME = 'grid-reaper';
export const ENV_VAR_BATCH_SIZE = 'BATCH_SIZE';
Copy link
Member

Choose a reason for hiding this comment

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

this is just a tiny bit confusing because I am assuming the variable name is ENV_VAR_BATCH_SIZE and the value is a number (string but number) but it sounds like the variable name is "BATCH_SIZE"? I could be reading wrong. But maybe clearer if we just use the string in place?

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 is a variable representing the name of the environment variable, and it's in the constants.ts so it can be referred to when it's set in the CDK stack, and then when its read in the lambda code. Having a constant for this avoids there ever being a discrepancy when using that value.

Copy link
Member

Choose a reason for hiding this comment

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

ok, I think I have a mild preference for using the string in place but I will leave it up to you, it's really not important :)


const recordDate = new Date();

const IDsToSoftDelete = await fetch(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const IDsToSoftDelete = await fetch(
const idsToSoftDelete = await fetch(

body: JSON.stringify(IDsToHardDelete),
},
).then(parseResponseAs<HardDeletedStatuses>());
// FIXME check for false values in the response
Copy link
Member

Choose a reason for hiding this comment

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

I think we still want to do this, or else change the 'Hard delete succeeded.' message?

.then((result) => {
const value = result.Parameter?.Value;
if (!value) {
throw Error(`Could not retrieve parameter value for '${Name}'`);
Copy link
Member

Choose a reason for hiding this comment

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

is there a useful message in result we could log here?

@twrichards
Copy link
Contributor Author

Closing in favour of a consolidated approach entirely within thrall - see #4145

@twrichards twrichards closed this Sep 20, 2023
@twrichards twrichards deleted the reaper/lambda-rewrite branch September 20, 2023 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants