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

Create EventAppliers in engine #9985

Merged
merged 1 commit into from Aug 8, 2022
Merged

Create EventAppliers in engine #9985

merged 1 commit into from Aug 8, 2022

Conversation

Zelldon
Copy link
Member

@Zelldon Zelldon commented Aug 4, 2022

Description

The StreamProcessor nor the builder doesn't need the knowledge about the EventAppliers. Currently there is no need to set this from outside.

Related issues

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/1.3) 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
  • If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Please refer to our review guidelines.

@Zelldon Zelldon requested a review from korthout August 4, 2022 09:37
@Zelldon Zelldon changed the title Create EventApplier in engine Create EventAppliers in engine Aug 4, 2022
Copy link
Member

@korthout korthout left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Zelldon
Copy link
Member Author

Zelldon commented Aug 4, 2022

bors r+

zeebe-bors-camunda bot added a commit that referenced this pull request Aug 4, 2022
9985: Create EventAppliers in engine r=Zelldon a=Zelldon

## Description

The StreamProcessor nor the builder doesn't need the knowledge about the EventAppliers. Currently there is no need to set this from outside.

<!-- Please explain the changes you made here. -->

## Related issues

<!-- Which issues are closed by this PR or are related -->




Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
@zeebe-bors-camunda
Copy link
Contributor

Timed out.

@Zelldon
Copy link
Member Author

Zelldon commented Aug 4, 2022

bors r+

zeebe-bors-camunda bot added a commit that referenced this pull request Aug 4, 2022
9985: Create EventAppliers in engine r=Zelldon a=Zelldon

## Description

The StreamProcessor nor the builder doesn't need the knowledge about the EventAppliers. Currently there is no need to set this from outside.

<!-- Please explain the changes you made here. -->

## Related issues

<!-- Which issues are closed by this PR or are related -->




Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
@zeebe-bors-camunda
Copy link
Contributor

Timed out.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022

Test Results

   706 files     706 suites   1h 37m 46s ⏱️
5 228 tests 5 205 ✔️ 11 💤 9  3 🔥
5 405 runs  5 382 ✔️ 11 💤 9  3 🔥

For more details on these failures and errors, see this check.

Results for commit 79bf2bc.

♻️ This comment has been updated with latest results.

@Zelldon
Copy link
Member Author

Zelldon commented Aug 5, 2022

There some tests failing because the need to set the event applies as mocks, we can resolve this with #9989 since we can set the RecordProcessor from outside (as spy for example) to verify things we do in the tests.

The StreamProcessor nor the builder doesn't need the knowledge about the EventAppliers. Currently there is no need to set this from outside.
@Zelldon
Copy link
Member Author

Zelldon commented Aug 8, 2022

bors r+

zeebe-bors-camunda bot added a commit that referenced this pull request Aug 8, 2022
9985: Create EventAppliers in engine r=Zelldon a=Zelldon

## Description

The StreamProcessor nor the builder doesn't need the knowledge about the EventAppliers. Currently there is no need to set this from outside.

<!-- Please explain the changes you made here. -->

## Related issues

<!-- Which issues are closed by this PR or are related -->




Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
@zeebe-bors-camunda
Copy link
Contributor

Timed out.

@Zelldon
Copy link
Member Author

Zelldon commented Aug 8, 2022

bors r+

zeebe-bors-camunda bot added a commit that referenced this pull request Aug 8, 2022
9985: Create EventAppliers in engine r=Zelldon a=Zelldon

## Description

The StreamProcessor nor the builder doesn't need the knowledge about the EventAppliers. Currently there is no need to set this from outside.

<!-- Please explain the changes you made here. -->

## Related issues

<!-- Which issues are closed by this PR or are related -->




Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
@zeebe-bors-camunda
Copy link
Contributor

Build failed:

@Zelldon
Copy link
Member Author

Zelldon commented Aug 8, 2022

bors r+

zeebe-bors-camunda bot added a commit that referenced this pull request Aug 8, 2022
9985: Create EventAppliers in engine r=Zelldon a=Zelldon

## Description

The StreamProcessor nor the builder doesn't need the knowledge about the EventAppliers. Currently there is no need to set this from outside.

<!-- Please explain the changes you made here. -->

## Related issues

<!-- Which issues are closed by this PR or are related -->




Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
@Zelldon Zelldon merged commit 155de10 into main Aug 8, 2022
@Zelldon Zelldon deleted the zell-clean-up-abstractions branch August 8, 2022 12:38
@zeebe-bors-camunda
Copy link
Contributor

Build failed:

Zelldon added a commit that referenced this pull request Aug 8, 2022
…ons"

This reverts commit 155de10, reversing
changes made to 47c3d3a.
zeebe-bors-camunda bot added a commit that referenced this pull request Aug 8, 2022
9753: Document events and event scopes r=korthout a=korthout

## Description

<!-- Please explain the changes you made here. -->
This provides an initial description of events and event scopes. It uses both the BPMN specification as well as the terminology used in Zeebe to describe events, triggers, and event scopes.

## Related issues

<!-- Which issues are closed by this PR or are related -->

NA


10021: Engine abstraction code cleanup r=pihme a=pihme

## Description
* Removes unnecessary references
* Moves `LegacyTypedResponseWriter` to stream processor, as it is only used in this package
* Renames `LegacyTypedResponseWriter`

## Related issues

related to #9727 



10028: Revert "Merge pull request #9985 from camunda/zell-clean-up-abstracti… r=Zelldon a=Zelldon

## Description

This reverts commit 155de10, reversing
changes made to 47c3d3a.
<!-- Please explain the changes you made here. -->

## Related issues

<!-- Which issues are closed by this PR or are related -->

related to #10027



Co-authored-by: Nico Korthout <nico.korthout@camunda.com>
Co-authored-by: pihme <pihme@users.noreply.github.com>
Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
Zelldon added a commit that referenced this pull request Aug 9, 2022
@Zelldon Zelldon mentioned this pull request Aug 9, 2022
10 tasks
zeebe-bors-camunda bot added a commit that referenced this pull request Aug 15, 2022
10036: Create new StreamPlatformExtension r=Zelldon a=Zelldon

## Description

This is an attempt to create a new StreamPlatformExtension, which should allow us to replace the TestStreams, StreamProcessorComposite, and StreamProcessorRule. This is something that would help us to continue with cleaning up the engine/stream processing code (and tests). There was a recent attempt to remove the event applies from the stream processing interface, #9985 but this failed since several tests have high dependencies on how the engine and related stuff is built. They are highly coupled.

This is just a first step and I would like to validate with you `@saig0` and `@npepinpe,` since I think you both are quite familiar with JUnit 5 extensions and our old rules. (Note: I'm a total junit 5 noob please bare with me.)

_What the PR does:_

 - Introduce a new class StreamPlatform (combination of [TestStreams](https://github.com/camunda/zeebe/blob/main/engine/src/test/java/io/camunda/zeebe/engine/util/TestStreams.java) and [StreamComposite](https://github.com/camunda/zeebe/blob/main/engine/src/test/java/io/camunda/zeebe/engine/util/StreamProcessingComposite.java))
 - Introduce a new extension to create the StreamPlatform and make this available for tests
 - Created an example test which is a copy of [StreamProcessorReplayModeTest.java](https://github.com/camunda/zeebe/blob/main/engine/src/test/java/io/camunda/zeebe/engine/processing/streamprocessor/StreamProcessorReplayModeTest.java)

In the test class we can see how it would look like and what the stream platform could provide us. Instead of always registering mocked typed processors in streamProcessors tests we just have on mocked RecordProcessor which can be use for verifications. I think this would simplify our tests a lot, since it is currently a lot of boilter plate which is mostly for the tests not interesting at all.

Be aware that it currently only contains one test to show how it could look like and get early feedback. 
Please take a look and let me know what you think.
<!-- Please explain the changes you made here. -->

## Related issues

<!-- Which issues are closed by this PR or are related -->

closes #



Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
zeebe-bors-camunda bot added a commit that referenced this pull request Aug 15, 2022
10036: Create new StreamPlatformExtension r=Zelldon a=Zelldon

## Description

This is an attempt to create a new StreamPlatformExtension, which should allow us to replace the TestStreams, StreamProcessorComposite, and StreamProcessorRule. This is something that would help us to continue with cleaning up the engine/stream processing code (and tests). There was a recent attempt to remove the event applies from the stream processing interface, #9985 but this failed since several tests have high dependencies on how the engine and related stuff is built. They are highly coupled.

This is just a first step and I would like to validate with you `@saig0` and `@npepinpe,` since I think you both are quite familiar with JUnit 5 extensions and our old rules. (Note: I'm a total junit 5 noob please bare with me.)

_What the PR does:_

 - Introduce a new class StreamPlatform (combination of [TestStreams](https://github.com/camunda/zeebe/blob/main/engine/src/test/java/io/camunda/zeebe/engine/util/TestStreams.java) and [StreamComposite](https://github.com/camunda/zeebe/blob/main/engine/src/test/java/io/camunda/zeebe/engine/util/StreamProcessingComposite.java))
 - Introduce a new extension to create the StreamPlatform and make this available for tests
 - Created an example test which is a copy of [StreamProcessorReplayModeTest.java](https://github.com/camunda/zeebe/blob/main/engine/src/test/java/io/camunda/zeebe/engine/processing/streamprocessor/StreamProcessorReplayModeTest.java)

In the test class we can see how it would look like and what the stream platform could provide us. Instead of always registering mocked typed processors in streamProcessors tests we just have on mocked RecordProcessor which can be use for verifications. I think this would simplify our tests a lot, since it is currently a lot of boilter plate which is mostly for the tests not interesting at all.

Be aware that it currently only contains one test to show how it could look like and get early feedback. 
Please take a look and let me know what you think.
<!-- Please explain the changes you made here. -->

## Related issues

<!-- Which issues are closed by this PR or are related -->

closes #



Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
zeebe-bors-camunda bot added a commit that referenced this pull request Aug 15, 2022
10036: Create new StreamPlatformExtension r=Zelldon a=Zelldon

## Description

This is an attempt to create a new StreamPlatformExtension, which should allow us to replace the TestStreams, StreamProcessorComposite, and StreamProcessorRule. This is something that would help us to continue with cleaning up the engine/stream processing code (and tests). There was a recent attempt to remove the event applies from the stream processing interface, #9985 but this failed since several tests have high dependencies on how the engine and related stuff is built. They are highly coupled.

This is just a first step and I would like to validate with you `@saig0` and `@npepinpe,` since I think you both are quite familiar with JUnit 5 extensions and our old rules. (Note: I'm a total junit 5 noob please bare with me.)

_What the PR does:_

 - Introduce a new class StreamPlatform (combination of [TestStreams](https://github.com/camunda/zeebe/blob/main/engine/src/test/java/io/camunda/zeebe/engine/util/TestStreams.java) and [StreamComposite](https://github.com/camunda/zeebe/blob/main/engine/src/test/java/io/camunda/zeebe/engine/util/StreamProcessingComposite.java))
 - Introduce a new extension to create the StreamPlatform and make this available for tests
 - Created an example test which is a copy of [StreamProcessorReplayModeTest.java](https://github.com/camunda/zeebe/blob/main/engine/src/test/java/io/camunda/zeebe/engine/processing/streamprocessor/StreamProcessorReplayModeTest.java)

In the test class we can see how it would look like and what the stream platform could provide us. Instead of always registering mocked typed processors in streamProcessors tests we just have on mocked RecordProcessor which can be use for verifications. I think this would simplify our tests a lot, since it is currently a lot of boilter plate which is mostly for the tests not interesting at all.

Be aware that it currently only contains one test to show how it could look like and get early feedback. 
Please take a look and let me know what you think.
<!-- Please explain the changes you made here. -->

## Related issues

<!-- Which issues are closed by this PR or are related -->

closes #



Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
@Zelldon Zelldon mentioned this pull request Aug 19, 2022
18 tasks
@Zelldon Zelldon added the version:8.1.0 Marks an issue as being completely or in parts released in 8.1.0 label Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
version:8.1.0-alpha5 Marks an issue as being completely or in parts released in 8.1.0-alpha5 version:8.1.0 Marks an issue as being completely or in parts released in 8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants