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

Async for each handling #16

Merged
merged 6 commits into from
May 19, 2020
Merged

Conversation

wcrooy
Copy link
Contributor

@wcrooy wcrooy commented May 7, 2020

This PR contains the possibility to handle multiple records at the same time. Based on mem-usage the degree of parallelism can be set.

Let me know if you've got any further questions or remarks.

@Kralizek
Copy link
Owner

Kralizek commented May 7, 2020

Hey @wcrooy!

Thank you for your PR. It's a really nice addition. I wonder whether these extensions should be in a side-package that can be added on the side. What do you think?

@wcrooy
Copy link
Contributor Author

wcrooy commented May 7, 2020

Hey @wcrooy!

Thank you for your PR. It's a really nice addition. I wonder whether these extensions should be in a side-package that can be added on the side. What do you think?

Hi @Kralizek.
You're welcome. I'm ok with both options. Though I think it's pretty close to the standard sqs / sns packages. Since it offers the user two flavours of initialize the processing of messages and notifications.

@wcrooy
Copy link
Contributor Author

wcrooy commented May 13, 2020

Hi @Kralizek ,
Is there anything I can do in assisting to get this PR through? Please let me know your thoughts.

Regards, Wouter

@Kralizek
Copy link
Owner

Hi @wcrooy, sorry. just busy these days. Is it urgent for you?

@Kralizek Kralizek self-requested a review May 13, 2020 08:00
@wcrooy
Copy link
Contributor Author

wcrooy commented May 13, 2020 via email

@Kralizek
Copy link
Owner

I'll try to work on it sometimes today. It generally looks really good.

@Kralizek
Copy link
Owner

@wcrooy do you think it's a correct assumption to say that if maxDegreeOfParallelism == 1 then we're basically running the sync version!

If so, we could merge the two extensions into one.

public static IServiceCollection UseNotificationHandler<TNotification, THandler>(this IServiceCollection services, int maxDegreeOfParallelism = 1)
    where TNotification : class
    where THandler : class, INotificationHandler<TNotification>
{
    if (maxDegreeOfParallelism <= 0)
    {
        throw new ArgumentOutOfRangeException(nameof(maxDegreeOfParallelism), $"{nameof(maxDegreeOfParallelism)} can't be less than 1");
    }

    if (maxDegreeOfParallelism > 1)
    {
        services.AddSingleton(new ForEachAsyncHandlingOption { MaxDegreeOfParallelism = maxDegreeOfParallelism });
        services.AddTransient<IEventHandler<SNSEvent>, SnsForEachAsyncEventHandler<TNotification>>();
    }
    else
    {
        services.AddTransient<IEventHandler<SNSEvent>, SnsEventHandler<TNotification>>();
    }

    services.AddTransient<INotificationHandler<TNotification>, THandler>();

    return services;
}

@Kralizek
Copy link
Owner

@wcrooy please review the changes I introduced in the latest commit. I think it makes a clearer API.

This is the idea:

// Non-parallel execution (implicit)
services.UseNotificationHandler<CustomNotification, CustomNotificationHandler>();

// Non-parallel execution (explicit)
services.UseNotificationHandler<CustomNotification, CustomNotificationHandler>(enableParallelExecution: false);

// Parallel execution, max parallelism set to available CPU count
services.UseNotificationHandler<CustomNotification, CustomNotificationHandler>(enableParallelExecution: true);

// Parallel execution, max parallelism set to specific value
services.ConfigureSnsParallelExecution(4);
services.UseNotificationHandler<CustomNotification, CustomNotificationHandler>(enableParallelExecution: true);

Similarly for SQS.

Copy link
Contributor Author

@wcrooy wcrooy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! Thanks! I like them. :-)

@Kralizek Kralizek merged commit 8a14c67 into Kralizek:master May 19, 2020
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 this pull request may close these issues.

2 participants