-
Notifications
You must be signed in to change notification settings - Fork 340
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
Customize DaprClient from workflow registration #1411
base: master
Are you sure you want to change the base?
Customize DaprClient from workflow registration #1411
Conversation
…tration Signed-off-by: Whit Waldo <[email protected]>
Signed-off-by: Whit Waldo <[email protected]>
ServiceLifetime lifetime = ServiceLifetime.Singleton) | ||
{ | ||
if (serviceCollection == null) | ||
{ | ||
throw new ArgumentNullException(nameof(serviceCollection)); | ||
} | ||
|
||
serviceCollection.AddDaprClient(lifetime: lifetime); | ||
|
||
if (configureDaprClient is not null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is probably ok, I wonder if, in this specific situation, the developer couldn't just call AddDaprClient()
themselves before calling AddDaprWorkflow()
(as the registration can only happen once)? This would save every derivative registration (e.g. workflow, AI, pub/sub, etc.) from having to add the complexity of additional lambdas in their APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to be able to have users install the various packages potentially independently of one another. If I want to connect to my local Dapr runtime for pubsub, but connect to a remote Dapr runtime for state management, that wouldn't be trivially done by having the developer call AddDaprClient()
as it would supersede the per-client options.
Perhaps in the shorter term then, instead of what I've got here, I need to refactor it all to output a DaprConnectionOptions
or the like and within each of the clients, stand up an unregistered instance of DaprClient
using those options.
Then, if you want the DaprClient
(as you would for most of today's Dapr functionality), you use AddDaprClient()
and if you want any of the other clients, you use AddWhateverClient()
, but the underlying DaprClient
s themselves are discrete from one another.
In the short term, it ensures you don't have lifecycle conflicts across the board. In the really long term, it means we can eventually drop AddDaprClient()
altogether once all the existing functionality is migrated into the purpose-built clients.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking a little more about this - today, because all the clients use static builders, it's insufficient to rely on the previously created DaprClient instances to get the configuration as the builders are recreating the connections from scratch.
So while the configuration bits may be available in the IConfiguration
, that's not necessarily going to cover all configuration scenarios (e.g. setting their own timeout, specifying their own gRPC options or their own JSON serialization settings), it'd at least handle setting endpoints (HTTP and gRPC) and the API token.
Description
While building out a proof of concept, I realized that there's no way to programmatically change anything about the DaprClient during first-time registration of a DaprWorkflowClient. Sure, if an envvar is set or the value is available from IConfiguration, the DaprClient will fallback to that, but if the developer wants to specify the values in their registration logic, the DaprClient defaults were used during registration.
This PR adds an optional argument to allow the user to pass a configuration delegate not unlike what they'd add to register the DaprClient from Dapr.Client.
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: N/A
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: