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

Added option to override GrpcChannelOptions when adding DaprWorkflow (#7218) #1244

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

humandigital-ruud
Copy link

@humandigital-ruud humandigital-ruud commented Feb 23, 2024

Description

You can now use custom GrpcChannelOptions when using DaprWorkflow, in the same manner as was already the case when creating the DaprClient:

builder.Services.AddDaprWorkflow(options =>
{
    options.UseGrpcChannelOptions(new GrpcChannelOptions
    {
        MaxReceiveMessageSize = 32 * 1024 * 1024,
        MaxSendMessageSize = 32 * 1024 * 1024
    });
});

Issue reference

See issue: GRPC data receive limits (#7218)

Copy link
Collaborator

@philliphoff philliphoff left a comment

Choose a reason for hiding this comment

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

A couple of comments about how gRPC options are exposed to both workflow runtime and client callers. Also, it'd be good to have some unit/integration tests built around at least the main scenario this is intended to solve (i.e. increasing the max. message size).

@philliphoff
Copy link
Collaborator

@humandigital-ruud Are you still interested in pursuing this PR?

@humandigital-ruud
Copy link
Author

@humandigital-ruud Are you still interested in pursuing this PR?

I will discuss this with my co-worker to see if he can take this over from me and spend some more time on it to get it right

@humandigital-michiel
Copy link

@humandigital-ruud Are you still interested in pursuing this PR?

I will discuss this with my co-worker to see if he can take this over from me and spend some more time on it to get it right

Hi!

@humandigital-michiel
Copy link

humandigital-michiel commented Jul 18, 2024

hi @philliphoff i made some adjustments based on your feedback. One of my first contributions to dapr.

@philliphoff gentle reminder for this one.

This commit refactors the DaprWorkflowClientBuilderFactory and WorkflowRuntimeOptions classes.

In DaprWorkflowClientBuilderFactory:
- Added a new method, UseGrpcChannelOptions, to allow the use of custom GrpcChannelOptions for creating the GrpcChannel.
- Updated the UseGrpc method to use the GrpcChannelOptions provided by the WorkflowRuntimeOptions.

In WorkflowRuntimeOptions:
- Added a new property, GrpcChannelOptions, to store the custom GrpcChannelOptions.
- Added a new method, UseGrpcChannelOptions, to set the GrpcChannelOptions.

These changes improve the flexibility and customization options for the Dapr workflow client.

Signed-off-by: Michiel van Praat <michiel.vanpraat@humandigital.nl>
@humandigital-michiel
Copy link

Refactored the changes.
We need to do some internal review, because fundamentals changed.
There is no unit test anymore on Workflow, because these we're removed some time ago, do you want these back?

Copy link
Contributor

@WhitWaldo WhitWaldo left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@WhitWaldo WhitWaldo dismissed philliphoff’s stale review October 30, 2024 03:35

Review created prior to refactoring

@WhitWaldo WhitWaldo added this to the v1.15 milestone Oct 30, 2024
@WhitWaldo
Copy link
Contributor

@humandigital-michiel This looks good on my end, but I'm showing the branch is out of date. Could you please merge the latest from master so I can complete this PR?

Thanks for putting this together!

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

Successfully merging this pull request may close these issues.

4 participants