Added option to override GrpcChannelOptions when adding DaprWorkflow (#7218) #1244
Conversation
| return GrpcChannel.ForAddress(daprEndpoint, options); | ||
| } | ||
| GrpcChannelOptions options = grpcChannelOptions ?? new(); | ||
| options.HttpClient ??= client; |
There was a problem hiding this comment.
A nit: if the caller provides gRPC options with a HttpClient, they won't get the Dapr API token behavior. If the caller does not provide a HttpClient, they'll only get access to "Dapr's" default HttpClient (after the fact, by virtue of it being attached to the options object). Neither is great if the caller needs a more customized HttpClient.
While this is changed is patterned after the options handling in the DaprClient (for which I have the same nit), I wonder if it might be good to try to use a pattern that allows callers to change options but not at the risk of overriding customized behavior needed by Dapr. A pattern that can then be replicated in DaprClient. Is there any reason to not just expose a public GrpcChannelOptions property from WorkflowRuntimeOptions? This property can be initialized to those settings typically used by Dapr (including the HttpClient). The caller then has the ability to add/change settings as needed with less risk of stomping on Dapr-needed settings.
There was a problem hiding this comment.
please review again, changed the way the parameters are used.
| @@ -153,31 +149,33 @@ static bool TryGetGrpcAddress(out string address) | |||
| return false; | |||
| } | |||
|
|
|||
| static GrpcChannel CreateChannel(string address, HttpClient client) | |||
| static GrpcChannel CreateChannel(string address, HttpClient client, GrpcChannelOptions? grpcChannelOptions = null) | |||
There was a problem hiding this comment.
This is also used by workflow clients, right? Would they not have similar needs to customize gRPC options as well?
There was a problem hiding this comment.
Hey @philliphoff this has been addressed couple of weeks ago, could you take a look? Many thanks!
There was a problem hiding this comment.
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).
|
@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
force-pushed
the
master
branch
2 times, most recently
from
4df8853
to
777a11b
Compare
|
hi @philliphoff i made some adjustments based on your feedback. One of my first contributions to dapr. @philliphoff gentle reminder for this one. |
…(#7218) Signed-off-by: Ruud van Falier <ruud.vanfalier@humandigital.nl> Signed-off-by: Michiel van Praat <michiel.vanpraat@humandigital.nl>
Signed-off-by: Michiel van Praat <michiel.vanpraat@humandigital.nl>
humandigital-michiel
force-pushed
the
master
branch
from
777a11b
to
b3a368a
Compare
Description
You can now use custom GrpcChannelOptions when using DaprWorkflow, in the same manner as was already the case when creating the DaprClient:
Issue reference
See issue: GRPC data receive limits (#7218)