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

Iterate over Side effects #9723

Closed
3 tasks
Zelldon opened this issue Jul 7, 2022 · 5 comments · Fixed by #10355
Closed
3 tasks

Iterate over Side effects #9723

Zelldon opened this issue Jul 7, 2022 · 5 comments · Fixed by #10355
Labels
kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc.

Comments

@Zelldon
Copy link
Member

Zelldon commented Jul 7, 2022

Description

Part of #9600

Side effects have been added in the early days when we had processing and reprocessing. In order to not redo certain actions, like sending requests or responses on reprocessing (all records have been reprocessed before) we introduced something we called SideEffects. The side effects have been executed only after commands have been processed.

After we migrated to "real" event stream processing only commands are processed (where side effects happen) and events are applied directly. On restart we reapply the events, so no commands are "reprocessed". This means we in general don't need that for the case of preventing sending requests or responses again.

One use case, which we might still have/need is to send requests / response only if the processing is successful and done. We need to clarify this.

Todo:

  • Clarify that use case or question whether we still want to support executing side effects after successful processing only Engine abstraction - Investigate side effects #9686
    • If yes, we might want to improve the current side effects to make it not so instable like it is right now (if a side effect is added it is likely the previous is overwritten). We maybe want to support a queue
    • If no, remove the side effects and send requests / responses directly

Additional Information

Take a look at the POC #9602

@Zelldon Zelldon added kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. team/distributed labels Jul 7, 2022
@Zelldon Zelldon mentioned this issue Jul 7, 2022
71 tasks
@pihme
Copy link
Contributor

pihme commented Jul 7, 2022

Summary of Discussion

These are the side effects currently produced by the engine:

  • CloseMessageSubscriptionSideEffectProducer
  • CloseProcessMessageSubscriptionSideEffectProducer
  • CorrelateMultipleProcessMessageSubscriptionsSideEffectProducer
  • FlushResponseWriterSideEffectProducer
  • OpenMessageSubscriptionSideEffectProducer
  • OpenProcessMessageSubscriptionSideEffectProducer
  • ScheduleBackOffSideEffectProducer
  • ScheduleTimerSideEffectProducer
  • SendCorrelateCommandSideEffectProducer
  • TypedResponseWriterImpl

Out of those, the following side effects need to happen after the transaction is committed:

  • ScheduleTimerSideEffectProducer (if timer is triggered before the information about the timer is committed to DB, this will produce an error)
  • TypedResonseWriterImpl/FlushResponseWriterSideEffectProducer - a response must only be sent if the processing was successful

For the other side effects, they may be run immediately during processing. It is expected that the engine will be able to deal with any scenarios that might arise, if the side effect got executed immediately, but then the commit failed.

Agreed Upon Design

  • The platform will provide a scheduling service that, among others, allows to schedule a task to run after successful commit of the current transaction
  • The engine will use this scheduling service to schedule what now are side effects
  • It is yet to be determined whether we need to do error handling on the scheduled tasks

@Zelldon
Copy link
Member Author

Zelldon commented Jul 8, 2022

Question from @npepinpe after POC:

#9602 (comment)
Side effects: how to guarantee that a side effect (which may be submitted in between processing) does not affect the state/buffers? It should run in an isolated sandbox in that case.

As you can see here #9723 (comment) we will provide with #9730 an way to run certain jobs/tasks after the commit of the current transaction/processing. This means we will be able to handle such issue as well.

@Zelldon
Copy link
Member Author

Zelldon commented Sep 7, 2022

Hey Team 👋

as written here https://camunda.slack.com/archives/C03HW6MG3Q9/p1661498640887859 I wanted to discuss this topic again. I think it becomes more important to help also to resolve issues like #10240 (Sorry I have only read the issue partially but I think it is related)

For people who can access the slack message:

Right now we execute such scheduled Tasks only if there is currently no command processing going on, this needs to be evaluated whether we want to keep it like that. I would like to get some input here, maybe
@Philipp Ossler
has some. This is also interesting for Iterate over Side-Effects. The question would be: Do we really need that? Are the new PostCommitTask in the ProcessingResult good enough, do we want to keep them?


Description:

As we can see in our discussion and outcome here is that we wanted to replace the Side-Effects with our ProcessingSchedulingService. During my, vacation Peter introduce a new interface for the side effects, which describes better what they do they are executed only after the transaction commit. It is called PostCommitTask and can be added to the ProcessingResult. The execution is done in the execute side effects.

When implementing the ProcessingScheduleService and certain guarantees, like running only after committed transaction it felt quite hacky and somehow not clean/optimal. In order to achieve this property we need to reschedule tasks, if currently processing is going on, which has of course certain side effects. Like jobs are rescheduled maybe multiple times etc..

Right now we don't really use it/need that guarantee.

My question is: Do we want to keep the PostCommitTasks as they are right now, which already guarantee us that they are executed AFTER commiting and remove the guarantee from the SchedulingService?

Benefit:

  1. This will reduce complexity of the SchedulingService code, and make it easier to reason about
  2. It will potentially help to resolve issues like IllegalStateException in StreamProcessor : Cannot complete future, the future is already completed #10240
  3. As a further improvement we could completely out source the SchedulingService to another Actor, such that we no longer block the StreamProcessor Actor. We just need to make sure that the SchedulingService consumer only have Readonly DB access and write commands to change state. This is anyway right now our pattern.

Happy to get feedback @npepinpe @deepthidevaki (since you were part of the #10240 discussion) and from @saig0 @korthout (since it is up to you whether you are fine with more lax guarantees of the processing schedule service and are ok with using the PostCommitTasks).

@saig0
Copy link
Member

saig0 commented Sep 13, 2022

Do we want to keep the PostCommitTasks as they are right now, which already guarantee us that they are executed AFTER commiting and remove the guarantee from the SchedulingService?

Yes. 👍 As far as I can see, the PostCommitTasks works for the engine. And we don't need the additional guarantee (i.e. avoid execution during the processing) of the SchedulingService.

We may see already issues with the guarantee of the SchedulingService because the scheduled tasks may be executed after a bunch of records are processed. The onCommit callback uses the fast lane in the actor scheduler to read new records. This could postpone the scheduled tasks until no new records are committed.

@Zelldon
Copy link
Member Author

Zelldon commented Sep 13, 2022

Thanks Phil for the feedback 🙇‍♂️ then I will remove this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants