feat(batch): Async Processing of Records for for SQS Fifo

[arnabrahman]

Summary

SQS Fifo doesn't have an async processing feature. This PR adds the feature to process async records with SQS Fifo.

Changes

• This is a WIP PR.
• Introduced a mixin for all the common functionalities for SQS Fifo
• Mixins have limitations when using private/protected properties. I updated the functions for this, the only major change is shortCircuitProcessing is now private instead of protected. The rest of the private functions remain the same.
• Use the mixin for SqsFifoPartialProcessor, and refactor the class to cater to this
• A new class SqsFifoPartialProcessorAsync is created to handle the async processing.
• Change processPartialResponse & BatchProcessingOptions for SqsFifoPartialProcessorAsync
• Duplicate the same tests of SqsFifoPartialProcessor to SqsFifoPartialProcessorAsync (This part can be optimized with describe.each I believe)

Issue number: closes #3140

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

[arnabrahman]

This is the initial implementation of Async processing for FIFO. Using Mixin to decouple common logics. But I might be missing something, please point me to the right direction @dreamorosi

[dreamorosi]

Hi @arnabrahman, thanks for the PR!

I'll review this tomorrow afternoon. I'm currently focused on tomorrow's release and since I think we are still on early phases of the implementation I don't think it'll make it for tomorrow.

[arnabrahman]

@dreamorosi No worries, take your time 🙂

[github-actions[bot]]

No related issues found. Please ensure there is an open issue related to this change to avoid significant delays or closure.

[dreamorosi]

Hi @arnabrahman, sorry this is taking a while.

This pattern is completely new to me and so I'm taking some time to read about it and understand the impact and future maintainability.

[dreamorosi]

Hi @arnabrahman - I have a deadline for this year's re:Invent set for Wednesday afternoon. I'm planning on reviewing the PR and provide meaningful feedback by Thursday afternoon.

Sorry for the delay - you know I would not let this stall too long if I really didn't have to.

Thanks for understanding!

[arnabrahman]

@dreamorosi, i get it. There's no need to apologize 🙂

[arnabrahman]

@dreamorosi, is it ok if i work on other open issues, which are maybe a bit more straightforward than this? As this is in a holding state.

[dreamorosi]

Hi @arnabrahman, thanks for your patience and for the long wait.

I have spent some time looking at this and while the implementation makes sense and works, I am concerned about introducing this pattern in the project.

This is likely a limitation of my understanding of the pattern, despite having spent some time on it, but I don't understand why Mixins in TypeScript can't support protected or private fields. Based on what I was able to gather, TypeScript doesn't allow you to add access properties to Mixin's methods because it ultimately isn't able to represent them in its type definitions (ts(4094) microsoft/TypeScript#30355, microsoft/TypeScript#36060).

Based on these issues, this seems to be still a very much open discussion with different workarounds that all have tradeoffs with non-obvious implications (here's a good summary of them).

Historically, we've been pretty intentional and somewhat strict with the access patterns we use to define methods, and if anything we are only getting stricter with the gradual adoption of actual JavaScript private classifiers (i.e. #foo). This is primarily because we take breaking changes seriously, and in order to do this we must keep our public API as tight as we can.

Having everything public like in the SqsFifo class represents somewhat of a risk in this area since it's an implicit green light for customers to rely on these methods being publics. This is still true even if we consider the fact that we are using the _name prefix, and that the SqsFifo mixin is not supposed to be used directly.

Ultimately, I am not sure that 1/ the ambiguity derived by TypeScript being unsure about how to treat these in type definitions, and 2/ the precedent of adding ambiguous access patterns in the codebase are worth saving some line of code that we'd have to write if we were to represent these with more vanilla inheritance or abstract classes like we already do with other processors in this package - even if this means having some repetition in the code.

I wonder if we could give a try to write this without Mixins and see if the outcome is less problematic. What do you think?

[dreamorosi]

@dreamorosi, is it ok if i work on other open issues, which are maybe a bit more straightforward than this? As this is in a holding state.

Yes please, feel free to pick up any of the existing open issues.

[arnabrahman]

@dreamorosi Thanks for your insight on this. I will write this without mixin & give it a try.

[sonarqubecloud[bot]]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
21.1% Duplication on New Code

See analysis details on SonarCloud