serverlessworkflow / specification

Contains the official specification for the Serverless Workflow Domain Specific Language. It provides detailed guidelines and standards for defining, executing, and managing workflows in serverless environments, ensuring consistency and interoperability across implementations.
http://serverlessworkflow.io
Apache License 2.0
740 stars 146 forks source link

The 'kind' parameter for event seems unnecessary #212

Closed jensg-st closed 3 years ago

jensg-st commented 3 years ago

What would you like to be added:

The kind parameter of the Event Definition can be removed.

Why is this needed:

In the specification these are all of the places where an event might be referenced:

In each case, whether or not the referenced event will be produced or consumed is transparent via context. What purpose does kind serve? It could be used as a sort of type safety, but I think the real-life impact is to introduce unnecessary complexity, verbosity, and cause trivial errors.

tsurdilo commented 3 years ago

@jensg-st the idea behind this parameter are scenarios when event definitions are defined in separate json/yaml files. This allows runtimes to parse these event definitions and up-front use the info to set up appropriate resources for these events given their "kind". Being able to do this has proven itself very useful with knative for example where the produced/consumed definitions can be used to up-front create all needed triggers and sink bindings (before any workflows are executed). Hope this helps.

alankm commented 3 years ago

I don't know. If you split an event definition into a separate file, wouldn't one of the main benefits be to consolidate the definition in a single place where both producers and consumers could access it?

Like if I create workflow "A" that is triggered by some event "E", and I want to create another workflow "B" that generates event E in order to trigger workflow A. Then I could have both workflows reference my event definition file so that they'd stay in-sync and up-to-date if I made any changes.

But only if the event definition can be used in both contexts -- i.e. produced and consumed.

I can't see the value of making that distinction on the event definition.

jensg-st commented 3 years ago

Even with the triggers, you would set them up when you either have an event state at the start or when the workflow instance hits an event state. In any case you don't need to check if that is consumed or produced. You check the event state, get the event references and get the event information and "enable" your events when you execute/start that state (what "enable" means might be different for different implementations).

Or do you think an implementation should create triggers on the references and when an event comes in checks if it is actually needed? That would be not a very efficient implementation I guess. Because the events and if they trigger are workflow specific and not global.

tsurdilo commented 3 years ago

@alankm note that the event produced by workflow (cloudevents) would need to have a different (their own) "source" context attribute. Events produced by workflows cannot have the same source as consumed events from other event sources.

tsurdilo commented 3 years ago

@jensg-st the main idea of this when we implemented it was that runtimes could read the event definitions json/yaml without having to look at all workflow definitions that use it, and be able to easily determine which events are consumed from "outside" sources or produced by workflow (which is responsibility of the runtime really), again, without having to parse any workflow control flow logic. We found that that could help with setting up things like knative/custom resources up-front for all the workflows given their reusable event / function definitions.

In addition it would help with validation as to make sure that a "produced" event is not being referenced inside event state for example (help for linters).

The "kind"parameter is optional enum with default of "consumed", so in most cases you do not need to really deal with it anyways.

With what I hear is that these particular scenarios is not something you guys consider useful. Am I correct?

alankm commented 3 years ago

@tsurdilo Good point. I hadn't considered that.

Now it's becoming something worthy of a separate issue, but perhaps enhancing the design to support reusable event definitions would be a good idea as an improvement? Just tossing ideas around here, but maybe an event consumer could support wildcards in the source and producers inject source information separate from the event definition. Maybe by adding a source field to the ProducedEvent Definition?

Anyway, I don't want to hijack this issue to make it about something else.

jensg-st commented 3 years ago

"Kind" is not optional. It is set to consumed if not set by the user. I just thought that is not needed and I think in the future most of the implementations won't use it. In a different issue you mentioned the name to provide some context. If users have issue with separating consumed and produced events they could use the name like "TempConsumed". From an implementation point of view it is not useful in my opinion. I would argue that it is not important for the users either. But the impact of the field is so small that it can be safely ignored at this stage. I agree with @alankm that there should be a discussion about event definitions anyways and instead of multiple names, events should have multiple sources and wildcards for example.

tsurdilo commented 3 years ago

@alankm we are looking at best ways to utilize the cloudevents subscriptions (https://github.com/cloudevents/spec/blob/v1.0.1/subscriptions-api.md) for that. There are impls where the event source is mapped to an MQTT topic, and MQTT does support wildcards for example "onboarding/+/start"

@jensg-st from the language definition perspective "kind" is not a required property, so it does not have to be explicitly defined in the json/yaml. I understand that for you it may seem not usable, but there are impls to my knowledge already using it, so not sure. If you mention that the impact of the field is so small then I would ask why then is it a must to remove it? :) I do not see the big problem from the implementation side either honestly, especially when it can help linters identify user typos during parsing of the workflow markup.

jensg-st commented 3 years ago

I just think it is better to get rid of unused stuff. I can not see any need for it and I as I have explained before I don't think there will be ever a need for it except accidentally using it in a consumed context if it is a produced context. Additional linting with minimal benefit. We will just ignore that field. A cleaner spec is always nicer.

tsurdilo commented 3 years ago

@jensg-st we go through all opened issues during our weekly team meetings (starting back up next monday the 11th). just mentioning this because me having a different opinion than you does not mean you should close the issue or feel as your opinion is being disregarded. During meetings we can as team decide what should go in or not, so everyone has a chance to give their opinion. yes I am very opinionated like most of us are but this spec is a community effort so please don't let me discourage you from pushing with your ideas/changes :)

jensg-st commented 3 years ago

I'm not discouraged :) This one is just not big enough to argue. There are other, bigger ones coming ;)

On Wed, 6 Jan 2021 at 14:17, Tihomir Surdilovic notifications@github.com wrote:

@jensg-st https://github.com/jensg-st we go through all opened issues during our weekly team meetings (starting back up next monday the 11th). just mentioning this because me have a different opinion that you does not mean you should close the issue or feel as your opinion is being disregarded. During meetings we can as team decide what should go in or not, so everyone has a chance to give their opinion. yes I am very opinionated like most of us are but this spec is a community effort so please don't let me discourage you from pushing with your ideas/changes :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/serverlessworkflow/specification/issues/212#issuecomment-755067252, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACTAE67UFUZT7N6ZM6OKXWTSYPP33ANCNFSM4VT4W2KQ .

-- Jens Gerke Software Architect

Mobile: +61 405 485 503 <+61+405+485+503> Email: jens.gerke@vorteil.io Web: http://www.vorteil.io Address: Suite 45, 194 Varsity Parade, Varsity Lakes, QLD, 4227