Skip to content
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

Implement Bulk Publish functionality #1001

Merged
merged 2 commits into from
Jan 26, 2023
Merged

Conversation

yash-nisar
Copy link
Contributor

Signed-off-by: Yash Nisar yashnisar@microsoft.com

Description

Implement Bulk Publish functionality

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: #958

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@yash-nisar yash-nisar requested review from a team as code owners December 23, 2022 04:18
@codecov
Copy link

codecov bot commented Dec 23, 2022

Codecov Report

Merging #1001 (6274dd5) into master (76d4b68) will increase coverage by 0.22%.
The diff coverage is 92.98%.

@@            Coverage Diff             @@
##           master    #1001      +/-   ##
==========================================
+ Coverage   69.92%   70.15%   +0.22%     
==========================================
  Files         157      160       +3     
  Lines        5227     5284      +57     
  Branches      562      567       +5     
==========================================
+ Hits         3655     3707      +52     
- Misses       1439     1443       +4     
- Partials      133      134       +1     
Flag Coverage Δ
net6 70.07% <92.98%> (+0.24%) ⬆️
net7 70.07% <92.98%> (+0.24%) ⬆️
netcoreapp3.1 70.11% <92.98%> (+0.23%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Dapr.Client/DaprClient.cs 92.59% <ø> (ø)
src/Dapr.Client/BulkPublishEntry.cs 70.00% <70.00%> (ø)
src/Dapr.Client/DaprClientGrpc.cs 87.27% <97.29%> (+0.39%) ⬆️
src/Dapr.Client/BulkPublishResponse.cs 100.00% <100.00%> (ø)
src/Dapr.Client/BulkPublishResponseFailedEntry.cs 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@shubham1172
Copy link
Member

Hello @yash-nisar, did you get a chance to look into dapr/sig-sdk-spec#22? It's still something I am figuring out, but with implementations across SDKs we can make both the SIG specification and SDK PRs consistent.

@yash-nisar
Copy link
Contributor Author

yash-nisar commented Jan 2, 2023

Hey @shubham1172 , not yet. Will look at it and make the necessary modifications. However, I've mostly tried to be consistent with dapr/java-sdk#789. Thanks for bringing this up !

src/Dapr.Client/DaprClient.cs Outdated Show resolved Hide resolved
src/Dapr.Client/DaprClient.cs Outdated Show resolved Hide resolved
src/Dapr.Client/DaprClient.cs Show resolved Hide resolved
src/Dapr.Client/DaprClientGrpc.cs Outdated Show resolved Hide resolved
src/Dapr.Client/DaprClientGrpc.cs Outdated Show resolved Hide resolved
src/Dapr.Client/DaprClientGrpc.cs Show resolved Hide resolved
Copy link
Contributor

@halspang halspang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine, I think you just need to update the example README to state which subscriber needs to be running.

@halspang
Copy link
Contributor

Oh, and please fix the merge conflicts.

@yash-nisar yash-nisar force-pushed the bulk_publish branch 2 times, most recently from 61a4faf to bf9f966 Compare January 25, 2023 03:19
Closes dapr#958

Signed-off-by: Yash Nisar <yashnisar@microsoft.com>
@halspang halspang merged commit 1605ecd into dapr:master Jan 26, 2023
@yash-nisar yash-nisar deleted the bulk_publish branch January 27, 2023 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bulk Pubsub implementation in SDK
4 participants