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

Atomic produce and consume actions #677

Closed
cdavernas opened this issue Sep 13, 2022 · 34 comments · Fixed by #826
Closed

Atomic produce and consume actions #677

cdavernas opened this issue Sep 13, 2022 · 34 comments · Fixed by #826
Assignees
Labels
area: spec Changes in the Specification change: feature New feature or request. Impacts in a minor version change
Milestone

Comments

@cdavernas
Copy link
Member

What would you like to be added:
Atomic produce and consume actions, meaning splitting the actual event action into two new actions: produceEvent and consumeEvent.

Why is this needed:

  • Allows for consuming without producing
  • Allows for producing without consuming
  • Easier to understand
  • More ubiquitous
@cdavernas cdavernas added change: feature New feature or request. Impacts in a minor version change area: spec Changes in the Specification schema labels Sep 13, 2022
@ricardozanini
Copy link
Member

@cdavernas, can you evolve a little bit your description (maybe including an example)? Not sure if I understood. We already have the produceEventRef and consumeEventRef in the EventAction: https://github.com/serverlessworkflow/specification/blob/main/specification.md#EventRef-Definition

@cdavernas
Copy link
Member Author

cdavernas commented Sep 14, 2022

@ricardozanini We do, but the produceEventRef property is required: no way to just consume an event. I don't see why I should have to produce a bogus event just to consume one. I get the 'asyncOperation' rationale behind it, but it is too restrictive IMO, especially in an event-driven world where the consumption of an event is the topmost priority => the consumption of an event drives it all.

We could just mark that property as non-required, but then the actual objet gets confusing with different properties that do the same thing: reference an event.

Example of what I'm proposing:

...
actions:
- name: Notify User about completion
  eventRef:
    refName: MyProducedEvent
    kind: produce # *
    data: {
       ...
    }
- name: Do something else
  functionRef: a-test-function
- name: Wait for something to happen
  eventRef:
    refName: MyConsumedEvent
    kind: consume# *
    dataFilter: {
      ...
    }
...
  • Here we could:

a. Determine the action of the eventRef by consulting the eventDefinition's kind property => bad for schema, as we loose that discriminator for produceEvent and consumeEvent properties, which differ

b. Remove the (IMO useless) kind property on the eventDef, and move it to the eventRef object => easy for schema, as it is our discriminator property. Plus, why should I have to repeat the definition of the same event if I want to both consume and produce it?

@cdavernas
Copy link
Member Author

cdavernas commented Sep 14, 2022

I would as well propose to remove the non-ubiquitous, obsolete invoke property on the eventRef. There is no sync/async paradigm here, it's an abuse of language applied to a non-applicable "async function" concept.

If need be for that concept, you can achieve it yourself by assembling one produce event and one consume event actions.

@tsurdilo
Copy link
Contributor

Think the reason for this is the event definition "source" property. If its consumed event then this has to be set to match the source where to consume from. if it's produced then runtime has to set the source.

so kind is "kinda" hehe, needed imo unless we say its not used when producing this event which is imo worse user experience maybe. i am not sure in how much % of cases you would consume and produce the exact same event definition, can you show example?

@cdavernas
Copy link
Member Author

Think the reason for this is the event definition "source" property. If its consumed event then this has to be set to match the source where to consume from. if it's produced then runtime has to set the source.

Well, that was another of my concerns, actually. IMO, source should NOT be required when consuming. You could very well filter by source and/OR type.

@tsurdilo
Copy link
Contributor

@cdavernas
Copy link
Member Author

cdavernas commented Sep 14, 2022

i am not sure in how much % of cases you would consume and produce the exact same event definition, can you show example?

Well, it's not a personal use case, just a reasoning, but I guess something like: to consume a "Hello" event from peers, shape it, then route it down to another group of peers: only the data would have changed

@cdavernas
Copy link
Member Author

cdavernas commented Sep 14, 2022

source is required attribute by CE https://github.com/cloudevents/spec/blob/main/cloudevents/spec.md#required-attributes

@tsurdilo Yes, but we are speaking of consuming (not producing) events: it's not because the spec mandates that CE must have a source that we should force user to filter on it!!! Otherwise, we should also add specVersion and id.

@tsurdilo
Copy link
Contributor

I think event defs are cheap to define. we could either leave it as is and deal with the duplication thats currently a must, or add on event defs another enum type for kind maybe "dual" or something and then document that in source you would have to set both producers (since CE allows more than one). For this however we would also i guess need some sort of hook for the source produced by the runtime which to me seems not optimal.

@tsurdilo
Copy link
Contributor

tsurdilo commented Sep 14, 2022

i personally like the eventRef kind lookup from the event def referenced, idk, seems natural to describe imo
think it also helps with being able to switch out event defs if you want to consume from test sources for testing idk

@cdavernas
Copy link
Member Author

cdavernas commented Sep 14, 2022

@tsurdilo yeah maybe, it's not the point of my feature request anyways. I just think that there is no added value to know that an event is gonna be consumed or produced in other places than in the action, so IMHO it's an obsolete property. But then again, doesn't really matter either, so let's leave it if you guys think it adds readability ;)

@tsurdilo
Copy link
Contributor

ok :) from the eventRef perspective if its more clear to have "produceEventRef" and "consumeEventRef" or similar, yeah thats fine imo.

@fjtirado
Copy link
Collaborator

fjtirado commented Sep 14, 2022

Which is the difference between consumeEventRef and a not starting event state? In my opinion there is a duality there. I would propose to delete consumeEventRef and just keep produceEventRef.
But I guess this depends on he answer to the first question

@JBBianchi
Copy link
Member

JBBianchi commented Sep 14, 2022

I tend to agree, EventRef and Event definitions should be refactored.

There might be a "trap" there: even if in the case of produce and consume, we are talking about Cloud Events, the purpose of each "action" is not exactly the same.

In the case of produce, we want two things: 1. describe the type of event that will be produced (aka the JSON Schema of the said event at best, or at least a source and a type I guess), 2. the mecanism to build the data for the runtime to actually produce the events.

In the case of consume though, I guess it could be way looser. One could want to match any property of an incoming CE and then select/filter anything from that CE.

As for async or not, I quite agree with the fact it doesn't really make sense. I guess it's rather a concern of have sequential or parallel product/consume event actions with a proper correlation between the two.

@cdavernas
Copy link
Member Author

cdavernas commented Sep 14, 2022

One is an action, which is chainable, the other is a whole state.
I remember that @tsurdilo mentioned, when we had this discussion months ago, that doing what I propose would make the event state obsolete, but would require updating the start by configuring events there.

I dont see the problem to have both living happily together, though.

@ricardozanini
Copy link
Member

@tsurdilo I think we can drop the source required attribute even in consume because this attribute can be dynamic. For instance, GitHub API can produce an event like this:

- event:
     type: openpr
     source: /github.com/serverlessworkflow/specification/pulls/122
     kind: consumed

In this situation, a workflow author would just add a dummy value that won't mean anything, and the instance would filter it either by regexp in the source or type.

I'll read the whole thread later. This source-required attribute is something I needed to bring up for quite a while.

@ricardozanini
Copy link
Member

And being a required attribute in the CE spec doesn't matter to us in the workflow definition. We can add a remark in the spec warning users that it's a required attribute. But needless to say, the CE SDKs will do their job and fail while parsing. Or any other implementations of the CE spec would do.

@tsurdilo
Copy link
Contributor

tsurdilo commented Sep 14, 2022

another thing we could do is change the event def "source" to "consumedSource" or something and then say that if the event is produced the runtime would set it..thats fair too imo and then you can remove the "kind"

@cdavernas
Copy link
Member Author

Which is the difference between consumeEventRef and a not starting event state?

@fjtirado Actually, the difference resides in the fact that the eventState can handle multiple events at once, whereas you would have to go through a parallel action sequence with multiple eventRefs to achieve the same. So for multiple events consumed in parallel, I'd go for the eventState, otherwise I'd go for an atomic eventRef of type produce

@fjtirado
Copy link
Collaborator

Which is the difference between consumeEventRef and a not starting event state?

@fjtirado Actually, the difference resides in the fact that the eventState can handle multiple events at once, whereas you would have to go through a parallel action sequence with multiple eventRefs to achieve the same. So for multiple events consumed in parallel, I'd go for the eventState, otherwise I'd go for an atomic eventRef of type produce

Yes, but you can also use an eventState to consume just one event type. I think we should avoid two ways of doing the same thing.

@cdavernas
Copy link
Member Author

another thing we could do is change the event def "source" to "consumedSource" or something and then say that if the event is produced the runtime would set it..thats fair too imo and then you can remove the "kind"

I would not do that: it will complicate things and will need another property if you want to filter a consumed event by source, which you should definitly be able to do (but should not be forced to).

What I'm proposing regard that specific point is to make the source property entirely optional, like @ricardozanini suggested, and it's up to the runtime to deal with eventual requirements at that level, or just throw an exception. I would recommend runtimes to set the source of cloud events to a configurable default value (ex: https;//synapse.io/ or https://kogito.kie.org/) when not explicitly set by workflow designer.

@tsurdilo
Copy link
Contributor

got it. ok thanks for explaining, ok so you want to offload the source to runtime if its not set? go for it

@cdavernas
Copy link
Member Author

Yes, but you can also use an eventState to consume just one event type. I think we should avoid two ways of doing the same thing.

Yes, maybe. But I feel this is a side effect of proposed feature, that should be addressed only if and when this feature is addressed.

To summarize:

We have a construct that is an opiniated aggregate of a produce and a consume event actions. Opiniated as well is the order in which actions can be performed: produce then (possibly not) consume.

At the moment we only support:

  • Produce only
  • Produce, then consume

Because of this, we forbid perfectly normal and widely used events flow, such as:

  • Consume only
  • Consume, then produce
  • Consume, then consume
  • Any other combination / use case

What I propose would address that, with minor impact and cleaner/better readability has a result IMO.

@cdavernas
Copy link
Member Author

@tsurdilo @ricardozanini @fjtirado @JBBianchi What do you guys think? Can I open a PR?

@ricardozanini
Copy link
Member

I think the proposal we had a while ago to refactor the onEvents would also tackle this change, IIRC.

I'd go for it but try to avoid doing the same thing twice like @fjtirado pointed out to avoid jeopardizing people's understanding.

@fjtirado
Copy link
Collaborator

@tsurdilo @ricardozanini @fjtirado @JBBianchi What do you guys think? Can I open a PR?

I think we can discuss the details on the PR, yes. But I would like to insist in the current duplicity between consumeEventRef and Event state. you can perfectly acompplish consumeEventRef functionality with an Event state with just one onEvent, while the only way to publish an event is through produceEventRef. We need to addess that, in my opinion we eiter remove consumeEventRef or we remove Event state (and use consumeEventRef and parallel/swith state to achieve the same)

@ricardozanini ricardozanini self-assigned this Nov 23, 2023
@cdavernas cdavernas assigned cdavernas and unassigned ricardozanini Feb 15, 2024
fjtirado added a commit to fjtirado/specification that referenced this issue Feb 28, 2024
fjtirado added a commit to fjtirado/specification that referenced this issue Feb 28, 2024
fjtirado added a commit to fjtirado/specification that referenced this issue Feb 28, 2024
fjtirado added a commit to fjtirado/specification that referenced this issue Feb 28, 2024
fjtirado added a commit to fjtirado/specification that referenced this issue Feb 28, 2024
fjtirado added a commit to fjtirado/specification that referenced this issue Feb 28, 2024
fjtirado added a commit to fjtirado/specification that referenced this issue Feb 28, 2024
fjtirado added a commit to fjtirado/specification that referenced this issue Feb 28, 2024
fjtirado added a commit to fjtirado/specification that referenced this issue Feb 28, 2024
fjtirado added a commit to fjtirado/specification that referenced this issue Feb 28, 2024
fjtirado added a commit to fjtirado/specification that referenced this issue Feb 28, 2024
Signed-off-by: Francisco Javier Tirado Sarti <[email protected]>
@ricardozanini ricardozanini moved this from Todo to In Progress in Progress Tracker Mar 4, 2024
fjtirado added a commit to fjtirado/specification that referenced this issue Mar 8, 2024
fjtirado added a commit to fjtirado/specification that referenced this issue Mar 8, 2024
Signed-off-by: Francisco Javier Tirado Sarti <[email protected]>
fjtirado added a commit to fjtirado/specification that referenced this issue Mar 8, 2024
Signed-off-by: Francisco Javier Tirado Sarti <[email protected]>
fjtirado added a commit to fjtirado/specification that referenced this issue Mar 8, 2024
Signed-off-by: Francisco Javier Tirado Sarti <[email protected]>
cdavernas added a commit that referenced this issue Mar 20, 2024
[Fix #677] Atomic produce and consume actions
@github-project-automation github-project-automation bot moved this from In Progress to Done in Progress Tracker Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: spec Changes in the Specification change: feature New feature or request. Impacts in a minor version change
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants