Subscriptions: bi-di index on per subscription#7770
Merged
Conversation
cicoyle
reviewed
Jun 3, 2024
cicoyle
left a comment
cicoyle
left a comment
Contributor
There was a problem hiding this comment.
Few comments from me, but generally looks good 👍🏻
| Resiliency: resiliency.New(nil), | ||
| }), | ||
| pubsubAdapter: &daprt.MockPubSubAdapter{}, | ||
| outbox: outboxfake.New(), |
Contributor
There was a problem hiding this comment.
This is a nit, but upon looking at our naming convention in dapr, I see fake<x>. Can this be renamed to fakeoutbox? or mockoutbox?
Contributor
Author
There was a problem hiding this comment.
I think this is more correct the name follows the package hierarchy outboxfake "github.com/dapr/dapr/pkg/outbox/fake"
| } | ||
|
|
||
| func (c *ComponentStore) ListDeclarativeSubscriptions() []subapi.Subscription { | ||
| func (c *ComponentStore) ListSubscriptionsAppByPubSub(name string) []*NamedSubscription { |
Contributor
There was a problem hiding this comment.
Suggested change
| func (c *ComponentStore) ListSubscriptionsAppByPubSub(name string) []*NamedSubscription { | |
| func (c *ComponentStore) ListPubSubSubscriptions(name string) []*NamedSubscription { |
Contributor
Author
There was a problem hiding this comment.
I like this name as it is more explicit- We are listing subscriptions, which are either programmatic or declarative (i.e. app callback based), which are subscribed to the given pubsub name.
| } | ||
|
|
||
| func (c *ComponentStore) GetDeclarativeSubscription(name string) (subapi.Subscription, bool) { | ||
| func (c *ComponentStore) ListSubscriptionsStreamByPubSub(name string) []*NamedSubscription { |
Contributor
There was a problem hiding this comment.
Suggested change
| func (c *ComponentStore) ListSubscriptionsStreamByPubSub(name string) []*NamedSubscription { | |
| func (c *ComponentStore) ListPubSubSubscriptionsStreams(name string) []*NamedSubscription { |
This was referenced Jun 19, 2024
Adds SubscribeTopicEvents proto API which dynamically subscribes to pubsub topics based on dapr/proposals#52. This is a basic gRPC implementation of the API whereby, like Subscription hot-reloading today, subscribing to a topic will reload _every_ active subscription for the current daprd. In a future PR, reloading of Subscriptions will be granular to the specific pubsub topic. Stream subscriptions are also only active once daprd declares the application as both present and ready. Dynamic stream subscriptions should be active both whether a app is running or not, as well as whether it is ready or not. This will be addressed in a future PR. Signed-off-by: joshvanl <[email protected]>
healthz. Refactors pubsub machinery to allow for gRPC bi-directional subcription streaming when there is no application, or the application in unhealhty. dapr/proposals#52 Signed-off-by: joshvanl <[email protected]>
Signed-off-by: joshvanl <[email protected]>
Signed-off-by: joshvanl <[email protected]>
Index on per subscription so that streams or Subscription hot reloading events will only reload that specific subscription, rather than reloading _every_ subscription for that PubSub component. This dramatically reduces disruption to topic subscriptions for a given PubSub component. Signed-off-by: joshvanl <[email protected]>
Signed-off-by: joshvanl <[email protected]>
Signed-off-by: joshvanl <[email protected]>
JoshVanL
force-pushed
the
bi-di-subscriptions-index-sub
branch
from
June 20, 2024 15:43
3679fff to
99bafe8
Compare
JoshVanL
marked this pull request as ready for review
June 20, 2024 15:44
cicoyle
reviewed
Jun 20, 2024
| } | ||
|
|
||
| if err := a.processor.Subscriber().ReloadPubSub(req.GetPubsubName()); err != nil { | ||
| if err := a.processor.Subscriber().StartStreamerSubscription(key); err != nil { |
Contributor
There was a problem hiding this comment.
Can key ever be empty or nil and if so should we do a nil check before starting the streamer subscription? I only see key being returned here key := a.pubsubAdapterStreamer.StreamerKey(req.GetPubsubName(), req.GetTopic()) but am uncertain if it could cause issues if returned unset and used?
Contributor
Author
There was a problem hiding this comment.
No, key is just a string concatenation function of the given strings so it won't be nil or empty
dapr/pkg/runtime/pubsub/streamer/streamer.go
Line 167 in 47a5b3a
yaron2
approved these changes
Jun 20, 2024
AnnuCode
pushed a commit
to AnnuCode/dapr
that referenced
this pull request
Aug 7, 2024
* Subscriptions: bi-directional subscription & publish streaming. Adds SubscribeTopicEvents proto API which dynamically subscribes to pubsub topics based on dapr/proposals#52. This is a basic gRPC implementation of the API whereby, like Subscription hot-reloading today, subscribing to a topic will reload _every_ active subscription for the current daprd. In a future PR, reloading of Subscriptions will be granular to the specific pubsub topic. Stream subscriptions are also only active once daprd declares the application as both present and ready. Dynamic stream subscriptions should be active both whether a app is running or not, as well as whether it is ready or not. This will be addressed in a future PR. Signed-off-by: joshvanl <[email protected]> * Subscriptions: bi-directional subscription streaming- subscribe on no healthz. Refactors pubsub machinery to allow for gRPC bi-directional subcription streaming when there is no application, or the application in unhealhty. dapr/proposals#52 Signed-off-by: joshvanl <[email protected]> * Fix unit tests Signed-off-by: joshvanl <[email protected]> * Fix subscription allowed Signed-off-by: joshvanl <[email protected]> * Subscriptions: bi-di index on per subscription Index on per subscription so that streams or Subscription hot reloading events will only reload that specific subscription, rather than reloading _every_ subscription for that PubSub component. This dramatically reduces disruption to topic subscriptions for a given PubSub component. Signed-off-by: joshvanl <[email protected]> * Lock streamer when sending topic mesgae to stream connection Signed-off-by: joshvanl <[email protected]> * Log Info when a streaming subscription unsubscribes Signed-off-by: joshvanl <[email protected]> --------- Signed-off-by: joshvanl <[email protected]> Signed-off-by: Annu Singh <[email protected]>
jake-engelberg
pushed a commit
to jake-engelberg/dapr
that referenced
this pull request
Sep 20, 2024
* Subscriptions: bi-directional subscription & publish streaming. Adds SubscribeTopicEvents proto API which dynamically subscribes to pubsub topics based on dapr/proposals#52. This is a basic gRPC implementation of the API whereby, like Subscription hot-reloading today, subscribing to a topic will reload _every_ active subscription for the current daprd. In a future PR, reloading of Subscriptions will be granular to the specific pubsub topic. Stream subscriptions are also only active once daprd declares the application as both present and ready. Dynamic stream subscriptions should be active both whether a app is running or not, as well as whether it is ready or not. This will be addressed in a future PR. Signed-off-by: joshvanl <[email protected]> * Subscriptions: bi-directional subscription streaming- subscribe on no healthz. Refactors pubsub machinery to allow for gRPC bi-directional subcription streaming when there is no application, or the application in unhealhty. dapr/proposals#52 Signed-off-by: joshvanl <[email protected]> * Fix unit tests Signed-off-by: joshvanl <[email protected]> * Fix subscription allowed Signed-off-by: joshvanl <[email protected]> * Subscriptions: bi-di index on per subscription Index on per subscription so that streams or Subscription hot reloading events will only reload that specific subscription, rather than reloading _every_ subscription for that PubSub component. This dramatically reduces disruption to topic subscriptions for a given PubSub component. Signed-off-by: joshvanl <[email protected]> * Lock streamer when sending topic mesgae to stream connection Signed-off-by: joshvanl <[email protected]> * Log Info when a streaming subscription unsubscribes Signed-off-by: joshvanl <[email protected]> --------- Signed-off-by: joshvanl <[email protected]> Signed-off-by: Jake Engelberg <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Index on per subscription so that streams or Subscription hot reloading events will only reload that specific subscription, rather than reloading every subscription for that PubSub component. This dramatically reduces disruption to topic subscriptions for a given PubSub component.
Branched from #7757
dapr/proposals#52