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

Inline resiliency policies for PubSub Subscription #46

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

olitomlinson
Copy link

No description provided.

@olitomlinson olitomlinson changed the title In-line resiliency policies for PubSub Subscribers Inline resiliency policies for PubSub Subscribers Nov 18, 2023

## Overview

I propose that declarative and programmatic PubSub subscribers should be extended specify a resiliency policy which ONLY affects the subscriber in question.
Copy link
Member

Choose a reason for hiding this comment

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

Define subscriber in question please. This is very ambiguous.

The pubsub component + app ID is a single subscriber in question.

Essentially this would override any resiliency policy for the component with regards to that very specific sidecar instance!

If you also want to include something topic specific here, this becomes a huge undertaking (requires refactoring of components, completely changing the policies we have etc)

Loading policies that are component specific from an app to overload the resiliency configuration may be possible however!

Copy link
Author

Choose a reason for hiding this comment

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

Define subscriber in question please. This is very ambiguous.

Hmmm... Maybe 'subscription' rather than 'subscriber' would be better.

"I propose that declarative and programmatic PubSub subscriptions should be extended to include a resiliency policy which is applied only to the the inbound process of delivering the message from sidecar to app. The resiliency policy is scoped to just the subscription it was declared into / against"

Does that read better?

Copy link
Author

@olitomlinson olitomlinson Nov 20, 2023

Choose a reason for hiding this comment

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

I'm proposing a change which doesn't have to impact any of the existing component resiliency mechanics. It simply doesnt use the existing component policy (if one exists) and applies a more specialised policy (if one has been provided via dapr/subscribe or via a subscription yaml) at the time of creating the subscription onto the topic.

image

Copy link
Member

Choose a reason for hiding this comment

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

Yes but still a bit too ambiguous.

  1. Would these settings apply to all topics within this subscription?

Or 2) do you need to be able to configure different settings for each topic?

The latter would be an extraordinary amount of work, and could instead be accomplished by repeating 1) with a different app and configuration - but still using the same pubsub component.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't see this image earlier. As long as subscription specific resiliency configuration applies to all topics within that subscription this sounds good to me!

Copy link
Author

Choose a reason for hiding this comment

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

  1. No. Each subscription has the ability to provide its own unique policy, just for that subscription. They are not shared across each Topic.

The whole point of this proposal is about enabling each subscription to operate via its own specialised policy, if the component-level policy is not suitable.

I didn't see this image earlier.

Ive just created the diagram to aid our conversation :)

As long as subscription specific resiliency configuration applies to all topics within that subscription this sounds good to me!

No, unfortunately it doesn't apply to all Topics as a blanket policy, it only applies to the subscriptions that that have been configured with the inline policy.

Copy link
Member

Choose a reason for hiding this comment

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

I'll ping you in Discord because I think you are still misunderstanding what I'm asking :)

Copy link
Member

@berndverst berndverst Nov 20, 2023

Choose a reason for hiding this comment

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

I'm talking about this difference:

@app.route('/dapr/subscribe', methods=['GET'])
def subscribe():
    subscriptions = [
      {
        'pubsubname': 'pubsub',
        'topic': 'checkout',
        'routes': {
          'rules': [
            {
              'match': 'event.type == "order"',
              'path': '/orders'
            },
          ],
          'default': '/orders',
          'resiliency': {
           # Resiliency Config here
          }
        }
      },
      {
        'pubsubname': 'pubsub',
        'topic': 'checkout2',
        'routes': {
          'rules': [
            {
              'match': 'event.type == "order"',
              'path': '/orders2'
            },
          ],
          'default': '/orders2',
          'resiliency': {
           # SAME Resiliency Config here
          }
        }
      }
]
    return jsonify(subscriptions)

instead of

@app.route('/dapr/subscribe', methods=['GET'])
def subscribe():
    subscriptions = [
      {
        'pubsubname': 'pubsub',
        'topic': 'checkout',
        'routes': {
          'rules': [
            {
              'match': 'event.type == "order"',
              'path': '/orders'
            },
          ],
          'default': '/orders',
          'resiliency': {
           # Resiliency Config here
          }
        }
      },
      {
        'pubsubname': 'pubsub',
        'topic': 'checkout2',
        'routes': {
          'rules': [
            {
              'match': 'event.type == "order"',
              'path': '/orders2'
            },
          ],
          'default': '/orders2',
          'resiliency': {
           # DIFFERENT resiliency config here
          }
        }
      }
]
    return jsonify(subscriptions)

We can only accept one configuration per Pubsub component / Sidecar combination.

@olitomlinson olitomlinson changed the title Inline resiliency policies for PubSub Subscribers Inline resiliency policies for PubSub Subscription Nov 20, 2023
@berndverst
Copy link
Member

berndverst commented Nov 20, 2023

I can only support this with the following assumptions:

There can only be one resiliency policy per (pubsub component, sidecar instance) combination. That is one policy per subscribing sidecar for a given component.

The ability to further customize this on a per topic basis would be too complex and I cannot support that. We have no ability in runtime and Dapr components to configure retry behavior that is topic specific. This would require a huge refactor of all components.

Additionally, the following load order should apply:

  1. Global resiliency policies configured by component path or k8s namespace
  2. Overloaded by programmatic pubsub resiliency policy or declarative pubsub definition.

The programmatic policy would win for a given component if it is defined in both places.

@berndverst
Copy link
Member

berndverst commented Nov 20, 2023

So after more discussion on Discord, the goal really is to provide unique resiliency policies for the (appID, pubsubComponent, route, topic) combination it seems.

This likely is no possible in runtime (a runtime maintainer needs to correct me here). The only topic and route aware subscription code resides in components. I believe this would require a huge refactor of components.

And honestly, this cannot be started until dapr/components-contrib#1863 is completed.

If it can be done without touching components however - great!

@olitomlinson
Copy link
Author

So after more discussion on Discord, the goal really is to provide unique resiliency policies for the (appID, pubsubComponent, route, topic) combination it seems.

Yes this is exactly what I'm trying to achieve.

If it can be done without touching components however - great!

I'm a newbie to Go so I will preface that I could be wrong here, but I think there is a potential solution that wouldn't touch the components, just the runtime. I've added this potential solution to the proposal.

I would be greatful if a runtime maintainer could validate what I've outlined below :)

https://github.com/dapr/proposals/blob/609a84d1d1565431af3e165c0da83c31d493cac8/R-inline-resiliency-policies-for-pubsub-subscription.md#potential-implementation-on-the-surface-this-looks-like-it-could-work-but-not-validated

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.

2 participants