CUMULUS-3223: Fix failed granule stuck in queued

[jennyhliu]

Summary: Summary of changes

Addresses CUMULUS-3223 Granule status stuck in 'queued'

Changes

• Detailed list or prose of changes
• ...

PR Checklist

• Update CHANGELOG
• Unit tests
• Ad-hoc testing - Deploy changes and test manually
• Integration tests

[Jkovarik]

HI Jenny - this looks really good, I have a question I'd like to discuss before this getting merged re: the update to the redrive on the sqs lambda, the rest are just informational questions.

[Jkovarik]

This updates looks really good @jennyhliu thanks for addressing my initial concern. There are a couple of additional comments/concerns I feel are worth talking through, let me know what you think!

[Jkovarik]

Nit: we might want to spell this out a bit, it's not going to be entirely transparent to end users what this means, but it likely matters to integrators who've been working with the DLQ

[Jkovarik]

I'm confused re: what this has to do with reprocessing - if I'm reading this right and it's more about processing the execution event for a cumulus message, this should probably be something like:

Suggested change

	let reprocessFailed = true;
	let processedExecutionEventMessage = false;

and have the below assertion negated for readability. Thoughts?

[Jkovarik]

Or .... perhaps remove the conditional logic and instead do something like:

  await Promise.all(sqsMessages.map(async (message) => {
    let cumulusMessage;
    const executionEvent = parseSQSMessageBody(message);
    try {
      cumulusMessage = await getCumulusMessageFromExecutionEvent(executionEvent);
    } catch (error) {
      log.error(`Writing message failed: ${JSON.stringify(message)} on getting message from execution event`, error);
      return batchItemFailures.push({ itemIdentifier: message.messageId });
    }
    try {
      return await writeRecords({ ...event, cumulusMessage, knex });
    } catch (error) {
      log.error(`Writing message failed: ${JSON.stringify(message)})`, error);
      return sendSQSMessage(process.env.DeadLetterQueue, message);
    }
  }));

[Jkovarik]

Docs related to this are at https://docs.aws.amazon.com/lambda/latest/dg/with-sqs.html under Implementing partial batch responses

[Jkovarik]

🤔

This does result in the method under test throwing in the proxyquire mock, and validates the unit case, but does this change warrant a fully integrated unit test or a lambda-invocation integration test
to show something like a message with a granule that should succeed, one that failed on writeGranules and one that failed on getExecutionEvent do the right thing given a single execution?

[jennyhliu]

@Jkovarik I have addressed all your comments under e71f482

[Jkovarik]

One last comment on review - in cases where there's a mangled SQS message, we're still sending the entire batch back. That seems unlikely in most operational situations, but it's a legit corner case. This is an improvement in either case, so it's not going to hold up the PR, but might be worth considering.

[Jkovarik]

We have units covering this is what the function returns, however we don't have a test, in integration, that shows on getCumulusMessageFromExecutionEvent failure, the outputs map to the correct convention and the system does what we expect. I'm fine with the results of our discussion in the Arch meeting re: performance testing being outside the scope of this ticket.

[Jkovarik]

Great work Jenny! This looks good, thanks for addressing my concerns.