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

Make RelationEvent a singleton per relation id / per {Created,Joined,Changed,Departed,Broken} #1205

Closed
phvalguima opened this issue Apr 27, 2024 · 3 comments

Comments

@phvalguima
Copy link
Contributor

phvalguima commented Apr 27, 2024

Within the same hook context, there is not much value to have more than one RelationEvent at time that represents a given relation & lifecycle stage.

The main motivation: it is hard, as the developer, to imagine we may issue / run several times the same Relation*Event for the same relation, during the same hook. As the databag changes are only applied at the end of the hook, there is little value for keeping more than one event around.
This change would remove a lot of the event storms we see around some charms caused by too many deferrals in a small period of time.

Also considering previous discussions things #1125, #966, #935.

I think the simplest case is the peers type: there is no purpose of having, say, more than one RelationChangedEvent for a given peer relation - there is always only one relation. So, if we had, for example in the same hook:

  1. Peer relation hook cluster-changed starts: emit a RelationChangedEvent for that peer relation
  2. Any call to emit() should return that same object instead of creating a new one
  3. If defer() is called, then the event is deferred
  4. Any time later the event is called again (e.g. an emit() or a previous deferred event is re-issued): abandon the request as RelationChangedEvent already ran

Same can be applied to non-peer relations, with the fact we should consider a per relation-id singleton as well.

@tonyandrewmeyer
Copy link
Contributor

Hi @phvalguima - just to clarify here, you're not suggesting that the event object should be a singleton (ie. you get the same event object in the handler each time, when compared with is you would get True), you're wanting ops to skip emitting (but consider emitted) any deferred events that are for the same relation ID (and relation event type) as the current Juju hook?

@phvalguima
Copy link
Contributor Author

Discussed with @tonyandrewmeyer and he will review this bug with @benhoyt later.

@benhoyt
Copy link
Collaborator

benhoyt commented May 17, 2024

Per Madrid discussion, we believe this would be solved by prevent duplicate events in the deferral queue, which is captured more explicitly by #935 -- so we're going to re-open that issue and close this one.

@benhoyt benhoyt closed this as not planned Won't fix, can't repro, duplicate, stale May 17, 2024
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

No branches or pull requests

3 participants