Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to partial batch response #26

Open
Kralizek opened this issue Feb 3, 2023 · 7 comments · May be fixed by #29
Open

Add support to partial batch response #26

Kralizek opened this issue Feb 3, 2023 · 7 comments · May be fixed by #29

Comments

@Kralizek
Copy link
Owner

Kralizek commented Feb 3, 2023

Let's discuss here how to best address partial batch responses.

This issue was spawned by #25

@bartpio bartpio linked a pull request Feb 3, 2023 that will close this issue
@bartpio
Copy link

bartpio commented Feb 3, 2023

I drafted #29 with an approach that changes the two SQS event handlers (SqsEventHandler and ParallelSqsEventHandler) to implement IRequestResponseHandler<SQSEvent, SQSBatchResponse>, in addition to IEventHandler<SQSEvent>.

The IRequestResponseHandler<SQSEvent, SQSBatchResponse> flavor can be used by writing an entry point class of the form:

public class Function : RequestResponseFunction<SQSEvent, SQSBatchResponse>
{
    // …

    protected override void ConfigureServices(IServiceCollection services, IExecutionEnvironment executionEnvironment)
    {
        // no difference here
        services.UseQueueMessageHandler<TestMessage, TestMessageHandler>();
    }
}

When this flavor is used, exceptions thrown by the IMessageHandler implementation are caught, and reported to Lambda in the response payload.

When the original IEventHandler<SQSEvent> flavor is used, exceptions propagate to the Lambda runtime, as in the prior state. The updated SQS handler implementations include exception filters to help reuse code, while ensuring that the code flow remains unchanged from the prior state (contrast with catching and re-throwing) when partial batch support is not used.

I had another thought to write an EventResponseFunction that is sort of a hybrid between EventFunction and RequestResponseFunction, but this turned out to be disruptive and overcomplicated.

@Kralizek please let me know what you think!

@Kralizek
Copy link
Owner Author

Kralizek commented Feb 8, 2023

I find your solution quite elegant but also hard to discover.

Maybe adding another template might be enough to solve the discoverability issue.

@bartpio
Copy link

bartpio commented Feb 9, 2023

I added a lambda-template-sqs-partial-batch template. This does help discoverability a bit. It is tricky to make this feature highly discoverable, especially for users not yet aware of its existence on the AWS side. If you have any further ideas, I would be glad to implement.

@bartpio
Copy link

bartpio commented Feb 9, 2023

Maybe another layer of abstract functions for entry point classes like in e63e887 helps discoverability a little?

@bartpio
Copy link

bartpio commented Feb 21, 2023

A downside of e63e887 is that while it's not breaking, it introduces an additional convention to choose from when writing a function (e.g. for regular SQS functions, should we derive from SqsEventFunction or EventFunction<SQSEvent>? is there a recommended path?).

For partial batch functions in particular, I find deriving from a pre-supplied SqsBatchResponseFunction rather than RequestResponseFunction<SQSEvent, SQSBatchResponse> mildly appealing from a discoverability and memorization standpoint. But this is not really a compelling argument for introducing an additional convention and potentially changing the recommended convention across the board.

If you do like e63e887 I would of course be glad to add it to the PR along with any ideas you might have regarding documentation.

I am now thinking your idea of adding another template does adequately solve the discoverability issue. I appreciate your feedback so far and your time spent thinking about the issue!

@Kralizek
Copy link
Owner Author

Hi, sorry I disappeared.

I've been thinking a bit about the best way to go forward and I would like to hear your opinion.

Basically, I am thinking about going back to a single template, make the current template use the RequestResponse version by default and leave the existing components for backward compatibility and advanced use cases.

Then users can specify if they want to invalidate the whole batch or each item one by one when registering the message handler.

I haven't had the time to play around with it yet, but I was thinking about giving it a go by looking at what you did in #29, unless you want to give it a go yourself.

I'm sorry it's taking so long but I'm a bit swamped personally.

@bartpio
Copy link

bartpio commented Feb 22, 2023

No worries at all! We can go back to using a single template, using the RequestResponse version, effectively making this the default. We can do something roughly analogous to WithParallelExecution (like the ParallelSqsExecutionOptions part, not the registering services part) when registering the message handler, so that the user can elect to invalidate batch items one by one.

Invalidating the whole batch should be default, as:

  1. This lines up with defaults on the AWS λ trigger side.
  2. This matches our prior state.

I'm thinking the new template should return a nullable SQSBatchResponse, like this:

public class Function : RequestResponseFunction<SQSEvent, SQSBatchResponse?>
{

We can return null in the default configuration (where the user has not elected to invalidate batch items one by one), pending research and validation that returning null from RequestResponse functions doesn't break anything. Sound reasonable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants