suggestions to improve new SQS handler #3328
Conversation
| AutoCancel.apply(zuoraRequest), | ||
| cancelRequestsProducer, | ||
| ZuoraEmailSteps.sendEmailRegardingAccount( | ||
| new ZuoraEmailSteps( |
There was a problem hiding this comment.
this existing code was using an unnecessarily hard to represent type for functions, so I fixed it as we're reusing it
| parsedCallout <- parseRecord(record) | ||
| (zuoraCalloutRecord, apiToken) = parsedCallout | ||
| processor <- ProcessCalloutSteps.build().toTry | ||
| _ <- processor.execute(zuoraCalloutRecord, apiToken).toTry(()) |
There was a problem hiding this comment.
the main changes here are rather than having A calls B calls C calls D etc (as per the original lambda that you've based on), it has A calls B, C and D. This makes it easier to understand each function's purpose as it's coherent rather than having lots of "handle" and "process" type functions.
Now we have a "parse" then "build steps" then "execute".
If you like the idea but not the names feel free to tweak them.
| } | ||
| } catch { | ||
| case e: Exception => | ||
| logger.warn(s"Exception sending cancellation email for account ${callout.accountId}: ${e.getMessage}", e) |
There was a problem hiding this comment.
it was just leaving a warning in the logs but not actually failing the lambda/alarming if the email couldn't be sent.
I can see the logic in doing that, so that it would then not process the record again, but we do need an alarm in that case.
Worst case it might think it's failed to send the email and end up sending it 3? times.
Happy to go either way on it if you have a preference either way.
andrade0
merged commit 0ba9e46
into
feat/auto-cancel-event-driven-migration
andrade0
deleted the
feat/auto-cancel-event-driven-migration-suggestions
branch
based on #3281
Rather than going back and forth with a load of comments, I thought it would be easier all round if I turned my feedback into a PR, meaning that it's easier for me to get my head around the codebase, and it becomes less frustrating for you. Also it's possible to do async as I'm catching up on my hours from earlier in the day.
Treat it like any other PR suggestion - it's probably worth considering, but if you have objections or other changes then that's fine! If you think it's perfect as it is (I haven't tested it mind) then feel free to merge it directly into your branch.
Changes are mostly to use more idiomatic scala, and also to bring it up to more recent coding standards (this is literally the oldest lambda in the repo, so there are a fair few techniques that we don't use hanging around)
See what you think, and see inline comments.