-
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
#906 -Added methods in status API supports for saving and reading binary data #1116
base: master
Are you sure you want to change the base?
Conversation
…f byte arrays dapr#906 Signed-off-by: Divya Perumal <[email protected]>
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.
Just a small change to ReadOnlyMemory<byte>
from byte[]
.
…y<byte> Signed-off-by: Divya Perumal <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1116 +/- ##
==========================================
+ Coverage 66.45% 66.78% +0.32%
==========================================
Files 171 171
Lines 5750 5831 +81
Branches 624 635 +11
==========================================
+ Hits 3821 3894 +73
- Misses 1782 1788 +6
- Partials 147 149 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Unless you have strong thoughts/usability concerns, return type when fetching the binary data should match the input type when we save it.
@halspang, @philliphoff - Thanks for your comments and sorry for the delay. Please review. |
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.
Looks ok, except for the incomplete comments. I made some suggestions and I think you can copy a lot of the parameter comments from other methods.
ArgumentVerifier.ThrowIfNullOrEmpty(storeName, nameof(storeName)); | ||
ArgumentVerifier.ThrowIfNullOrEmpty(key, nameof(key)); | ||
ArgumentVerifier.ThrowIfNull(etag, nameof(etag)); | ||
return await this.MakeSaveByteStateCallAsync(storeName, key, ByteString.CopyFrom(value.Span), etag, stateOptions, metadata, cancellationToken); |
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.
Since you're not doing anything with the result of the awaited task you can optimize the code a bit by dropping the await
here and remove async
from the method signature
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 think this 'await' is needed as MakeSaveByteStateCallAsync is an asyc task. Similar to method-TrySaveStateAsync. Thanks.
Description
#906 -Added methods in status API supports for saving and reading binary data.
Added methods to directly save and read Byte arrays.
New methods defined in DaprClient.cs
New methods implemented in DaprClientGrpc.cs
Unit tests written in StateApITest.cs
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: #[906]
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: