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

Extract Streamprocessor from engine module #10979

Merged
merged 17 commits into from
Nov 14, 2022
Merged

Conversation

Zelldon
Copy link
Member

@Zelldon Zelldon commented Nov 11, 2022

Description

Split the engine module into two separate modules.

One called stream-platform (zeebe-stream-platform), the name is of course discussable, which contains everything related to stream processing. This module contains right now the API and implementation. It can and will be split further in the future.
There is a new package names io.camunda.stream.api/impl etc. Here we can also agree on a different name of course.

The other remaining module is still called engine, which contains the record processor implementation. It is the core or business logic of zeebe (the process engine). Right now it still depends on some impl classes like CopiedRecords, RecordValues mostly related to tests. This can hopefully be resolved in the future via iteration over the engine tests. #10004

I prepared some structure in the API and impl, but can also be discussed.

There are several classes which have been moved, which we might want
to move to a different place later.

For example:

  • ZbColumnFamily
  • CopiedRecords
  • RecordValues
  • etc.

Related issues

closes #10977
closes #10130
closes #9727
closes #9600

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.)
  • 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.

Split engine module into two separate modules.

One called stream-platform which contains everything related to stream
processing. This module contains right now the API and implementation.
It can and will be splitted further in the future.

The other remaining module is still called engine, which contains the
record processor implementation. It is the core or business logic of
zeebe (the process engine).

Implementation classes have been moved and imports are adjusted in this
commit.

There are several classes which needed to be moved, which we might want
to move to a different place later.

For example:

 * ZbColumnFamily
 * CopiedRecords
 * RecordValues
 * etc.
@Zelldon Zelldon changed the title Split engine module Extract Streamprocessor from engine module Nov 11, 2022
@Zelldon
Copy link
Member Author

Zelldon commented Nov 11, 2022

I guess it is best to just check the structure locally. 😁 Or via clicking . (dot) to open the web editor 😅

@github-actions
Copy link
Contributor

github-actions bot commented Nov 11, 2022

Test Results

   954 files  +  1     954 suites  +1   1h 46m 39s ⏱️ + 6m 11s
7 514 tests +21  7 507 ✔️ +21  7 💤 ±0  0 ±0 
7 713 runs  +21  7 704 ✔️ +21  9 💤 ±0  0 ±0 

Results for commit 0ffe34d. ± Comparison against base commit a8317c7.

♻️ This comment has been updated with latest results.

@npepinpe
Copy link
Member

+1,439 −995 ? Instant LGTM ;)

I'll try to have a look at it today assuming no medic stuff pops up 🙏

In order to decouple the engine from the implementation, we have to duplicate the NextValueManager. We can later simplify this to have maybe something else in use in the keygenerate to reduce the duplication.
@Zelldon
Copy link
Member Author

Zelldon commented Nov 14, 2022

@npepinpe @oleschoenburg @oleschoenburg someone of you time for a review today? To avoid too much merge conflicts with other PR's 😅

Copy link
Member

@npepinpe npepinpe left a comment

Choose a reason for hiding this comment

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

👍

bom/pom.xml Outdated Show resolved Hide resolved
engine/pom.xml Show resolved Hide resolved
*/
package io.camunda.zeebe.engine.state;
package io.camunda.zeebe.protocol;
Copy link
Member

Choose a reason for hiding this comment

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

💭 Yes, definitely, this should be moved somewhere else 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you already have an idea?

Copy link
Member

Choose a reason for hiding this comment

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

OTOH, it wouldn't be that crazy for the whole DB persistence layer to have its own module, e.g. engine-state or something, then that could go in there. I guess not something we can unilaterally decide. Or it can go in the API module once we have one? Since that will be a dependency for both the stream platform impl and the engine?

I guess in the stream platform module we just need it for the processed position and such things? We don't really access the engine states, right? It would be cool if we could somehow share the DB but not know about the engine state/column families either...like a way to extend the state? We open the DB, create our own state/columns, then pass it on to the engine. They also don't need to know about our own columns/states. Not sure how feasible that is due to how we index though :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah state module is something I also wanted since a while but still this wouldn't solve it since stream platform doesn't want to know all these details.

What I thought first I could extend an enum, first define the two columns and then later extend in the engine with more columns, but unfortunately enums can't be extended since they are final 😅

Theoretically, we could do something similar via composition I guess, we have some default columns defined on base level which always need to be defined. Call the class BaseColumns these are used by stream-platform, and engine defines own columns and also uses BaseColumns. 🤷 But I felt this is too much for this PR. :)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's fair. I think we should do it long term (both the state module and taking the composition approach, so each module doesn't need to know all columns).

stream-platform/pom.xml Outdated Show resolved Hide resolved
Zelldon and others added 2 commits November 14, 2022 15:18
Co-authored-by: Nicolas Pepin-Perreault <43373+npepinpe@users.noreply.github.com>
Co-authored-by: Nicolas Pepin-Perreault <43373+npepinpe@users.noreply.github.com>
@Zelldon
Copy link
Member Author

Zelldon commented Nov 14, 2022

bors r+

@zeebe-bors-camunda
Copy link
Contributor

Build succeeded:

@zeebe-bors-camunda zeebe-bors-camunda bot merged commit 4649a21 into main Nov 14, 2022
@zeebe-bors-camunda zeebe-bors-camunda bot deleted the zell-split-modules-2 branch November 14, 2022 15:21
zeebe-bors-camunda bot added a commit to camunda/zeebe-process-test that referenced this pull request Nov 14, 2022
565: Fix code after stream-engine module split r=remcowesterhoud a=Zelldon

## Description

Due to recent module split several imports have changed. This has to be adjusted and the event applies are no longer created outside.

Should only be merged AFTER camunda/camunda#10979 is merged
<!-- Please explain the changes you made here. -->

## Related issues

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

closes #

<!-- Cut-off marker
_All lines under and including the cut-off marker will be removed from the merge commit message_

## Definition of Ready

Please check the items that apply, before requesting a review.

You can find more details about these items in our wiki page about [Pull Requests and Code Reviews](https://github.com/camunda-cloud/zeebe/wiki/Pull-Requests-and-Code-Reviews).

* [ ] I've reviewed my own code
* [ ] I've written a clear changelist description
* [ ] I've narrowly scoped my changes
* [ ] I've separated structural from behavioural changes
-->

## Definition of Done

<!-- Please check the items that apply, before merging or (if possible) before requesting a review. -->

_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

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

Documentation:
* [ ] Javadoc has been written
* [ ] The documentation is updated


Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a StreamProcessor Module Clean up StreamingPlatform / Engine [EPIC]: Engine Abstraction
3 participants