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

Alternative proposal for Actor PubSub implementation #75

Closed
wants to merge 4 commits into from

Conversation

WhitWaldo
Copy link

None of the comments have been addressed in the various reviews on the existing Actors PubSub proposal nor has any discussion occurred on the thread I created in the maintainers channel on Discord.

But as there is voting occurring on the existing proposal, I'm making a last-ditch effort to suggest ditching that proposal in favor of an alternative that I think is more broadly applicable to what people want to get out of Actors and PubSub, repurposes what's already available and used in Dapr and avoids having additional "one component for all occasions" dependencies.

Fundamentally, I take what we already use for Dapr PubSub today (declarative, programmatic and streaming subscriptions) and extend it to Actors. Contrary to the existing proposal which changes how anyone would have to send events in to use them in actors, my proposal allows actors to subscribe to any existing PubSub component without changing how the events come in and only changing how actors themselves subscribe. In other words, mine is a drop-in change that anyone can use on day-one with existing PubSub without needing anything more than annotating actor methods with corresponding routing (for declarative or programmatic subscriptions).

I welcome any and all feedback!

@WhitWaldo
Copy link
Author

Pinging the following who have expressed interest in this proposal for visibility:
@JoshVanL @yaron2 @msfussell @elena-kolevska @olitomlinson @nelson-parente @cicoyle @theperm

Comment on lines +264 to +269
repeated oneof subscribe_actor_event_request_filter_type {
// Registers a filtered subscription with a CEL expression that must evaluate as true
SubscribeActorTopicEventsRequestFilterAlpha1
// Registers a non-filtered subscription
SubscribeActorTopicSinkAlpha1
} = 6;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the benefit of using a oneof here, rather than having expression as an optional?

Copy link
Author

Choose a reason for hiding this comment

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

No real benefit - just copied the original approach you had.

will subscribe to the specified queue/topic indicated. This topic string will take the format of:

```
dapr.actors||$namespace||$actorType
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that a namespace is required for all subscriptions?
Having namespace as optional can be both beneficial and detrimental to some systems, depends on use case of course.
Or it is assumed that both publishers and subscribers operate in the same namespace and actor type pubsub messages will be routed only within the originating namespace?

Copy link
Author

Choose a reason for hiding this comment

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

I simply followed the text from the original proposal here. I'd be inclined towards making it optional to favor more scenarios than fewer.

WhitWaldo and others added 3 commits March 25, 2025 02:30
Co-authored-by: Anton Troshin <[email protected]>
Signed-off-by: Whit Waldo <[email protected]>
Co-authored-by: Anton Troshin <[email protected]>
Signed-off-by: Whit Waldo <[email protected]>
Co-authored-by: Anton Troshin <[email protected]>
Signed-off-by: Whit Waldo <[email protected]>
@WhitWaldo
Copy link
Author

Following the maintainer call on 3/24, @JoshVanL and I talked through several aspects of this proposal and I wanted to write them down for posterity:

Shift from existing PubSub approach - single Dapr consumer instead of per subscriber

Where it makes sense for the PubSub broker to set up a new consumer for each subscription today, this isn't practical with PubSub actors as offhand, I cannot think of a PubSub provider that supports more than 32 consumer groups, much less an infinite amount (e.g. the potential identifier space for actor IDs). Rather, I propose that this be handled differently in that the Dapr runtime itself be the sole consumer of inbound messages and that it itself is responsible for attempting to locate a subscribed message consumer. If a consumer is identified and the message conveyed, the message should be acknowledged back to the PubSub component (and retries to subscribers handled per resiliency policies). If no consumer is found, the messages should be either retried or dropped (per policy).

Subscription Types

We agreed that the most valuable subscription type here is streaming. I expressed support for the notion that like Orleans, the concept of a PubSub subscription should be one where they're potentially either long-standing or brief and favor dynamic creation/deletion. To that end, I'd be entirely OK with dropping declarative subscriptions from the initial release. While it could be nice to have programmatic subscriptions (applied only at service startup and applicable only to previously activated actor instances), if the initial version only allows for streaming subscriptions, I think that'd be a great place to start.

Existing declarative PubSub cannot point to actors for lack of identifying actor names and instance IDs.

Filter Type

Today, we favor Google's CEL approach for handling event filtering, but it's inconsistent with the approach suggested in CloudEvents. While I modified the CloudEvent schema to support this CEL approach, we might want to take advantage of the opportunity here of a few feature to instead wholeheartedly embrace conformance with CloudEvents instead of CEL.

Subscription de-duplication

Because I propose that the filter be assigned on the runtime (to avoid unnecessary actor activation), in the short term this introduces the opportunity to do subscription de-duplication on a per-actor ID basis. Reiterating from the proposal, I think it's very possible that a single actor instance may have more than one subscription to the same PubSub component and topic (and potentially with the same filter across multiple methods) so there's an opportunity to at least put all these in the same record.

That said, I would eventually (as part of a separate proposal) support another Orleans capability of rewinding subscriptions. When a subscription is created, it naturally receives only the next message received, but if the underlying provider supports sequence tokens (e.g. like Azure Event Hub), these can be passed back by Dapr from the subscriber to pull earlier messages prior to subscription. This might have an impact on runtime-side de-duplication, so it's something to consider in advance.

Unmentioned on Call

There are a few additional tweaks I'll add to the PR itself, but since I'm thinking about it:

  • Should have a means of getting an existing subscription details (even if just by ID)
  • Should have a means of listing existing subscriptions for a given actor ID
  • Shift to supporting CloudEvents 1.0.2 standard
    This is sort of covered above in the note about CEL vs CloudEvent filtering, but worth repeating. Today, Dapr assumes that if the content type isn't provided, it must be text/plain and rather assumes that the data is JSON or similar for filtering. I would instead propose that instead of having our own funky hybrid of this standard, we take the opportunity to fully support CloudEvent including support for binary or text payloads on events. and adopt the subscriptions spec for this PubSub actor functionality.

@WhitWaldo
Copy link
Author

Closing in favor of #78 since I put this on the wrong local branch and can't change it

@WhitWaldo WhitWaldo closed this Mar 25, 2025
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.

3 participants