Add support for shutdown API and add client docs #922
Conversation
Codecov Report
@@ Coverage Diff @@
## master #922 +/- ##
==========================================
- Coverage 69.68% 69.63% -0.05%
==========================================
Files 155 155
Lines 5119 5121 +2
Branches 553 553
==========================================
- Hits 3567 3566 -1
- Misses 1421 1424 +3
Partials 131 131
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| /// </summary> | ||
| /// <param name="cancellationToken">A <see cref="CancellationToken" /> that can be used to cancel the operation.</param> | ||
| /// <returns>A <see cref="Task" /> that will return when the operation has completed.</returns> | ||
| public abstract Task ShutdownSidecarAsync(CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
nitpicking:
Do we generally try to keep the func/method name the same across SDKs?
I'm asking this because on go-sdk and python-sdk we named it as Shutdown rather than ShutdownSidecar[Async]
There was a problem hiding this comment.
This is something that we'll define in the sig-sdk group, but I do think it's important to keep naming consistent. I do worry that both of those are a little to lax on the name though.
Do you think it's worth being different here for clarity? Having the API just be Shutdown, to me, doesn't really tell the user what it's doing.
There was a problem hiding this comment.
I don't have a strong opinion here,
There are negative and positive effects of being explicit in this case, I do not feel much comfortable exposing the word Sidecar I would say that "ShutdownDaprd" sound better, and, if by any chance we would have more than one sidecar (probably won't happen) it becomes explicit which one is shutting down. But again, it's nitpicking and I agree with you that just Shutdown is too generic
There was a problem hiding this comment.
+1 to everything said here. This seems like a good chance to be consistent on the terminology.
This commit adds support for the shutdown API. It also adds docs for that method and a few others which were missing from the docs. dapr#914 Signed-off-by: Hal Spang <halspang@microsoft.com>
| await client.ShutdownAsync(new Empty(), CreateCallOptions(null, cancellationToken)); | ||
| } | ||
|
|
||
| #endregion |
Description
This commit adds support for the shutdown API. It also adds docs for
that method and a few others which were missing from the docs.
#914
Signed-off-by: Hal Spang halspang@microsoft.com
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: #914
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: