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

• A new class SqsFifoPartialProcessorAsync is created to handle the async processing.
• A small util class for SQS FIFO processing, if we agree on this then it can be used inside SqsFifoPartialProcessor
• Change processPartialResponse & BatchProcessingOptions for SqsFifoPartialProcessorAsync
• Run the same sets of tests for SqsFifoPartialProcessorAsync as we did for SqsFifoPartialProcessor
• Update the Fifo related docs for async processing with examples.

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 🙂

[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.

[am29d]

Thanks for PR! I have minor comments for clarification on the utility class. I also wonder if it would make sense to pull up a SqsFiroProcessor class with the utils functionality for both cases.

[sonarqubecloud]

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

[am29d]

Thank for the PR @arnabrahman, great work!

While I have an itch to reduce the code duplication in this PR, @dreamorosi and I agreed to move it to a separate task that would affect the entire batch utility. We could simplify a lot of functionality and have a better implementation for sync and asynchronous processors.