Initial Bulk Subscribe functionality by yash-nisar · Pull Request #1009 · dapr/dotnet-sdk

yash-nisar · 2023-01-12T03:36:34Z

Signed-off-by: Yash Nisar [email protected]

Description

Initial Bulk Subscribe implementation

Please explain the changes you've made

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

codecov · 2023-01-12T03:38:51Z

Codecov Report

Merging #1009 (72e426f) into release-1.10 (c863582) will decrease coverage by 0.51%.
The diff coverage is 42.30%.

@@               Coverage Diff                @@
##           release-1.10    #1009      +/-   ##
================================================
- Coverage         69.45%   68.95%   -0.51%     
================================================
  Files               162      169       +7     
  Lines              5432     5534     +102     
  Branches            585      588       +3     
================================================
+ Hits               3773     3816      +43     
- Misses             1519     1578      +59     
  Partials            140      140

Flag	Coverage Δ
net6	`68.86% <42.30%> (-0.52%)`	⬇️
net7	`68.86% <42.30%> (-0.52%)`	⬇️
netcoreapp3.1	`68.91% <42.30%> (-0.51%)`	⬇️

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

Impacted Files	Coverage Δ
src/Dapr.AspNetCore/BulkMessageModel.cs	`0.00% <0.00%> (ø)`
src/Dapr.AspNetCore/BulkSubscribeAppResponse.cs	`0.00% <0.00%> (ø)`
...c/Dapr.AspNetCore/BulkSubscribeAppResponseEntry.cs	`0.00% <0.00%> (ø)`
src/Dapr.AspNetCore/BulkSubscribeMessage.cs	`0.00% <0.00%> (ø)`
src/Dapr.AspNetCore/BulkSubscribeMessageEntry.cs	`0.00% <0.00%> (ø)`
src/Dapr.AspNetCore/BulkSubscribeTopicOptions.cs	`0.00% <0.00%> (ø)`
...NetCore/DaprEndpointConventionBuilderExtensions.cs	`30.30% <0.00%> (-8.16%)`	⬇️
src/Dapr.AspNetCore/BulkSubscribeAttribute.cs	`100.00% <100.00%> (ø)`
...r.AspNetCore/DaprEndpointRouteBuilderExtensions.cs	`94.24% <100.00%> (+1.14%)`	⬆️
src/Dapr.AspNetCore/Subscription.cs	`93.75% <100.00%> (+2.08%)`	⬆️
... and 1 more

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

halspang · 2023-01-25T00:12:44Z

I like that we were able to reuse the metadata here. Is it possible to make a more convenient function for this? Or a [BulkSubscribeTopicMetadata] that flows through the same path? I know we originally didn't want a new annotation but I was just thinking if we follow the path of the metadata but provide a convenient annotation for it.

Just one point here, bulk subscribe is no longer part of metadata, but it is its own field similar to route in subscriptions.

halspang · 2023-01-25T00:27:02Z

Nit, indentation doesn't need to be here.

halspang · 2023-01-25T00:40:00Z

How does this look as an actual payload coming in? My hope is that it would be an array of the actual events so we don't need the CloudEventModel in the middle as that's the point of this middleware is to remove the cloudevent.

Below we just reference the data field for example.

Discussed offline and addressed. The reason being, if we just reference data, we might not get other information like entryId. These fields might be important as they tell the consumer which message failed/passed.

halspang · 2023-01-25T00:42:37Z

Generally, publicly accessible fields should be capitalized also this should be Entries or something like that since you need the individual entry to get the status.

halspang · 2023-01-25T00:43:27Z

No data? Where is that stored?

https://github.com/dapr/components-contrib/blob/master/pubsub/responses.go#L35

halspang · 2023-01-25T00:44:01Z

What's the empty constructor for?

halspang · 2023-01-25T00:50:29Z

So this is the actual response back? Or what we send out? I was confused with how DaprBulkAppResponse didn't have any events.

DaprBulkAppResponse is the response that we send back to dapr.

mukundansundar · 2023-01-26T19:06:20Z

Just one point here, bulk subscribe is no longer part of metadata, but it is its own field similar to route in subscriptions.

mukundansundar · 2023-01-26T19:08:12Z

Suggested change

/// Response from the application containing status for each entry in the bulk message.

/// It is posted by the bulk subscribe handler.

/// Response from the application containing status for each entry in the bulk message.

/// It is posted to the bulk subscribe handler.

?

mukundansundar · 2023-01-26T19:09:51Z

ideally this enum can be used for normal subscribe response to, if there is a domain model for normal subscribe response ...

We don't have that yet.

halspang

Mostly just questions/formatting stuff.

Signed-off-by: Yash Nisar <[email protected]>

halspang · 2023-02-08T00:11:42Z

@mukundansundar - Can you validate this to make sure we aren't missing anything?

mukundansundar

bulk subscribe lgtm

Signed-off-by: Yash Nisar <[email protected]>

* Update DurableTask SDK dependency to get ARM64 compatibility (#1024) (#1025) * Update DurableTask SDK dependency to get ARM64 compatibility * Fix issue with gRPC address override behavior Signed-off-by: Chris Gillum <[email protected]> * Initial Bulk Subscribe functionality (#1009) Signed-off-by: Yash Nisar <[email protected]> * Workflow unit testing changes for 1.10 release (#1038) Signed-off-by: Chris Gillum <[email protected]> * Fix issue with gRPC address override behavior Signed-off-by: Chris Gillum <[email protected]> * Workflow SDK changes to enable unit testing Signed-off-by: Chris Gillum <[email protected]>

yash-nisar requested review from a team as code owners January 12, 2023 03:36

halspang requested changes Jan 25, 2023

View reviewed changes

mukundansundar reviewed Jan 26, 2023

View reviewed changes

halspang mentioned this pull request Jan 27, 2023

Workflow Management - Initial Methods #1003

Merged

3 tasks

yash-nisar force-pushed the bulk_subscribe branch 3 times, most recently from de99401 to 8e3a2d4 Compare February 6, 2023 17:19

yash-nisar requested review from halspang and mukundansundar and removed request for halspang and mukundansundar February 6, 2023 17:25

halspang reviewed Feb 6, 2023

View reviewed changes

yash-nisar force-pushed the bulk_subscribe branch from 8e3a2d4 to 8ea37fe Compare February 7, 2023 22:41

Initial Bulk Subscribe functionality

9f987d5

Signed-off-by: Yash Nisar <[email protected]>

yash-nisar force-pushed the bulk_subscribe branch from 8ea37fe to 9f987d5 Compare February 7, 2023 22:43

yash-nisar requested a review from halspang February 7, 2023 22:44

halspang changed the base branch from master to release-1.10 February 7, 2023 23:51

Merge branch 'release-1.10' into bulk_subscribe

72e426f

halspang approved these changes Feb 8, 2023

View reviewed changes

mukundansundar reviewed Feb 8, 2023

View reviewed changes

halspang merged commit 0f1e1bf into dapr:release-1.10 Feb 8, 2023

yash-nisar deleted the bulk_subscribe branch February 8, 2023 18:53

This was referenced Feb 8, 2023

Bulk Pubsub implementation in SDK #958

Closed

[EPIC] Make Bulk Pubsub APIs stable dapr/dapr#5567

Closed

yash-nisar added a commit to yash-nisar/dotnet-sdk that referenced this pull request Feb 27, 2023

Initial Bulk Subscribe functionality (dapr#1009)

5154f33

Signed-off-by: Yash Nisar <[email protected]>

yash-nisar added a commit to yash-nisar/dotnet-sdk that referenced this pull request Feb 27, 2023

Initial Bulk Subscribe functionality (dapr#1009)

cad3005

Signed-off-by: Yash Nisar <[email protected]>

-    /// Response from the application containing status for each entry in the bulk message.
-    /// It is posted by the bulk subscribe handler.
+    /// Response from the application containing status for each entry in the bulk message.
+    /// It is posted to the bulk subscribe handler.

Uh oh!

Conversation

yash-nisar commented Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue reference

Checklist

Uh oh!

codecov Bot commented Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halspang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

halspang commented Feb 8, 2023

Uh oh!

mukundansundar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yash-nisar commented Jan 12, 2023 •

edited

Loading

codecov Bot commented Jan 12, 2023 •

edited

Loading