add refreshing client sample #119
Conversation
src/RefreshingClient/Program.cs
Outdated
| var replaceWorkerClient = (TemporalClient newClient) => | ||
| { | ||
| worker.Client = newClient; | ||
| Console.WriteLine("Client's new handle: {0}", worker.Client.BridgeClientProvider?.BridgeClient?.DangerousGetHandle()); |
There was a problem hiding this comment.
okay to log this for the sample?
The idea was to show connection id changing, but don't know if this is the right field.
There was a problem hiding this comment.
I would rather not log this, and just say it was replaced. These internals are not considered stable or meant for users.
src/RefreshingClient/Program.cs
Outdated
| var replaceWorkerClient = (TemporalClient newClient) => | ||
| { | ||
| worker.Client = newClient; | ||
| Console.WriteLine("Client's new handle: {0}", worker.Client.BridgeClientProvider?.BridgeClient?.DangerousGetHandle()); |
There was a problem hiding this comment.
I would rather not log this, and just say it was replaced. These internals are not considered stable or meant for users.
There was a problem hiding this comment.
Hrmm, I wonder if a single client refresh after some time would be a better demonstration. I have not seen this sample in any other Core-based SDKs, so it is a bit strange to only be in .NET, but not a big deal. However, I do not want to encourage the practice of rotating a client every 10 seconds (many calls are up to a minute long anyways, so rotation may happen a few times before the client is even used). I fear the sample demonstrating this kind of frequent rotation may encourage it.
There was a problem hiding this comment.
yes, that is a valid point. I will change to rotating it every 2 hours for the sample.
And leave a comment for making it configurable.
There was a problem hiding this comment.
done, let me know if you still prefer the demonstration to be once instead of every 2 hours.
I do understand this is not a Workflow pattern, so happy to sample it elsewhere.
There was a problem hiding this comment.
This works for me, but realistically a user may never see the client refresh in the sample. Just want confirmation that's ok with you before merging.
|
|
||
| try | ||
| { | ||
| await Task.WhenAll(ClientRefreshAsync(replaceWorkerClient, tokenSource.Token), worker.ExecuteAsync(tokenSource.Token)); |
There was a problem hiding this comment.
Not sure this is worth breaking out into 2 more separate methods vs just inlining the loop here, but not a big deal
There was a problem hiding this comment.
This works for me, but realistically a user may never see the client refresh in the sample. Just want confirmation that's ok with you before merging.
What was changed
Added a project to demonstrate TemporalClient rotation/refresh
Why?
Have a customer needing to refresh client and rotate credentials every 2 hours.
Thought its a worth a sample
Checklist
Closes
How was this tested: