-
-
Notifications
You must be signed in to change notification settings - Fork 25
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
Comments
I drafted #29 with an approach that changes the two SQS event handlers ( The 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 When the original I had another thought to write an @Kralizek please let me know what you think! |
I find your solution quite elegant but also hard to discover. Maybe adding another template might be enough to solve the discoverability issue. |
I added a |
Maybe another layer of abstract functions for entry point classes like in e63e887 helps discoverability a little? |
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 For partial batch functions in particular, I find deriving from a pre-supplied 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! |
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. |
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 Invalidating the whole batch should be default, as:
I'm thinking the new template should return a nullable 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? |
Let's discuss here how to best address partial batch responses.
This issue was spawned by #25
The text was updated successfully, but these errors were encountered: