feat!: support partial batch responses on all handlers #75
Conversation
|
Merging this PR will result in a major version bump. Created by lifeomic-probot (Enforce Semantic Commits) |
swain
force-pushed
the
partial-batch-responses
branch
from
6c9a325 to
6a9ccce
Compare
swain
requested review from
nicolls1,
aecorredor,
SchaeStewart,
aymanenadi and
jkdowdle
| { | ||
| items: event.Records, | ||
| // If there is not a MessageGroupId, then we don't care about | ||
| // the ordering for the event. We can just generate a UUID for the | ||
| // ordering key. | ||
| orderBy: (record) => record.attributes.MessageGroupId ?? uuid(), | ||
| orderBy: (record: SQSRecord) => |
There was a problem hiding this comment.
Hm - I guess now that we're reusing this for all sources the record can't really be generic.
There was a problem hiding this comment.
This line is total cruft. The type here is inferred from event.Records. The explicit type is not needed.
| subsequentUnprocessedRecords: [ | ||
| expect.objectContaining({ | ||
| body: JSON.stringify({ | ||
| name: `test-event-4`, | ||
| }), | ||
| }), |
There was a problem hiding this comment.
We lost verifying that the subsequent unprocessed records are also reported, right?
There was a problem hiding this comment.
@aecorredor Unfortunately, it's slightly worse. We don't actually report subsequentUnprocessedRecords at all.
Thanks for catching it.
Would you mind if I follow-up with re-adding that improvement? Getting this fix out for the medical-results team is actually time-sensitive -- would love to have it before the weekend.
| subsequentUnprocessedRecords: [ | ||
| expect.objectContaining({ | ||
| body: JSON.stringify({ | ||
| name: `test-event-8`, | ||
| }), | ||
| }), | ||
| ], |
There was a problem hiding this comment.
Same here. Or am I missing something? I do see that you still track subsequent unprocessed records, so we could validate that in the test, right?
| ); | ||
|
|
||
| return { batchItemFailures }; | ||
| return handleUnprocessedRecords({ |
There was a problem hiding this comment.
I do think the new approach is simpler - but I felt like it'd be nice for this methods to not have to be another import, since we always want to call it after calling processWithOrdering right? Either to decide if we should throw or if we should accept partial batching.
There was a problem hiding this comment.
Throwing something out there, maybe the return type of processWithOrdering makes is just a method that is called to do what handleUnprocessedRecords is doing. That way things are more direct.
There was a problem hiding this comment.
@aecorredor I had the same feeling. I think that could be a nice follow-up improvement.
|
Super nice! I liked that not a whole lot of changes were needed to make this work. Your new abstraction is also simpler 🚀 |
|
🎉 This PR is included in version 6.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Motivation
Currently, we only support partial batch responses for our SQS handler. But, all of the event sources we support (Kinesis, Dynamo, SQS) actually support using partial batch responses.
The medical results team would like to use Kinesis partial batch responses. So, this PR adds support for partial batch responses in all of our handlers.
I went ahead and used a breaking change just for sanity.