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

Jump to bottom

Apply event-sourcing to the process instance subscription correlate processor #6558

Merged
1 commit merged into from Mar 17, 2021
Merged

Apply event-sourcing to the process instance subscription correlate processor #6558

1 commit merged into from Mar 17, 2021

Conversation

saig0
Copy link
Member

@saig0 saig0 commented Mar 16, 2021 •
edited

Description

  • align the processor's name with the intent
    • smaller improvements of the maintainability
    • but the changes look more than they are because of the renaming from CorrelateProcessInstanceSubscription to ProcessInstanceSubscriptionCorrelateProcessor
  • move the state changes in the new event applier
  • extract the bootstrapping of the pending subscription checker
  • cancel/pause the subscription checker if the processing is paused or failed
    • adopted the behavior of other listeners

Related issues

closes #6182

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

  • The changes are backwards compatibility with previous versions
  • If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/0.25) to the PR, in case that fails you need to create backports manually.

Testing:

  • There are unit/integration tests that verify all acceptance criterias of the issue
  • New tests are written to ensure backwards compatibility with further versions
  • The behavior is tested manually
  • The change has been verified by a QA run
  • The impact of the changes is verified by a benchmark

Documentation:

  • The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
  • New content is added to the release announcement

@saig0 saig0 force-pushed the 6182-subscription-correlate branch from eb36b18 to e6354dd Compare March 16, 2021 07:46
@saig0 saig0 marked this pull request as ready for review March 16, 2021 07:46
@saig0 saig0 requested a review from pihme March 16, 2021 07:46
pihme
pihme suggested changes Mar 16, 2021
View reviewed changes
Copy link
Contributor

@pihme pihme left a comment

Choose a reason for hiding this comment

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

I think there is one mistake in it.

And I have a question and a comment.

engine/src/main/java/io/zeebe/engine/processing/common/EventHandle.java
@@ -25,52 +25,66 @@
private final KeyGenerator keyGenerator;
private final MutableEventScopeInstanceState eventScopeInstanceState;

// TODO (saig0): use immutable states only (#6200)
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI when working on Job.ThrowError I opted not to use this class again, I split it up into the parts that modify state and parts that don't and had to refactor the logic so much, that it didn't resemble well the existing class. Especially, because Job.ThrowError is using error event handle, so there was more logic to look at.

Same is currently true for the work on timer processors. We pretty much inlined the methods of this class into the timer processor and then migrated the inlined version.

This may be questionable in terms of DRY, but has some benefits in terms of reducing dependencies.

There is stuff in JobThrowErrorProcessor which could be harvested as part of #6200. And I guess we could make a deliberate decision whether we want to keep this class. If so, we need some refactoring in JobThrowError processor to use the migrated class again.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is okay to use this class for now because it doesn't modify the state here effectively. In the end, I would like to avoid duplication. But let's do it as part of #6200.

...n/java/io/zeebe/engine/processing/message/ProcessInstanceSubscriptionCorrelateProcessor.java Outdated Show resolved Hide resolved
...c/main/java/io/zeebe/engine/state/appliers/ProcessInstanceSubscriptionCorrelatedApplier.java
public void applyState(final long key, final ProcessInstanceSubscriptionRecord value) {

if (value.isInterrupting()) {
subscriptionState.remove(value.getElementInstanceKey(), value.getMessageNameBuffer());
Copy link
Contributor

Choose a reason for hiding this comment

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

Question for my understanding: How do the other partitions know that the subscription was removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

The message subscription of the other partition has the same property. The processor closes the message subscription when sending the correlate command.

@saig0 saig0 requested a review from pihme March 16, 2021 14:16
@saig0
Copy link
Member Author

saig0 commented Mar 16, 2021

@pihme thanks 🍰 I applied your suggestion. Please have a look.

@saig0
chore(engine): apply event-sourcing to the correlate processor
e77bc10 
* align the processor's name with the intent
* move the state changes in the new event applier
* extract the bootstrapping of the pending subscription checker
* cancel/pause the subscription checker if the processing is paused or failed
@saig0 saig0 force-pushed the 6182-subscription-correlate branch from b92eae0 to e77bc10 Compare March 17, 2021 06:07
@saig0
Copy link
Member Author

saig0 commented Mar 17, 2021

bors r+

@ghost
Copy link

ghost commented Mar 17, 2021

Build succeeded:

@ghost ghost merged commit fda31cc into develop Mar 17, 2021
@ghost ghost deleted the 6182-subscription-correlate branch March 17, 2021 06:32
@pihme pihme mentioned this pull request Apr 16, 2021
Merged
9 tasks
ghost pushed a commit that referenced this pull request Apr 21, 2021
@pihme
merge: #6811
1ae55de 
6811: Improve time handling in random processes r=pihme a=pihme

## Description

- adds information to AbstractExecutionStep whether the step is automatic or can be controlled in execution
- adds information to AbstractExecutionStep how much execution time the step will consume
- adds new class ScheduledExecutionStep to decorate an execution step with time and scheduling information
- changes the way that variables are computed; they can now be overridden after the full execution path is known. In particular, this is done to set the timeouts of boundary timer events based on their effective activation time. For a description of the concept, look at #6558
- adds boundary timer events to Service Task and Sub Process as a proof of concept
- adds some quality of life improvements like printing out at which point in the execution a failure occurred

Recommended way to review this:
- read about concept in #6568 
- scan over the history of commits (not so much to review each commit, but more to get a sense of the overall zig zag)
- check out branch and review end product

## Related issues

closes #6568 



Co-authored-by: pihme <pihme@users.noreply.github.com>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pihme pihme pihme approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Apply event-sourcing to the workflow instance subscription processors
4 participants
@saig0 @pihme @npepinpe @menski