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.

Sign up for GitHub

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

Move Atomix log storage implementation into the broker #8975

Closed
npepinpe opened this issue Mar 24, 2022 · 3 comments · Fixed by #9235
Closed

Move Atomix log storage implementation into the broker #8975

npepinpe opened this issue Mar 24, 2022 · 3 comments · Fixed by #9235
Assignees
@Zelldon
Labels
area/maintainability Marks an issue as improving the maintainability of the project kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. version:8.1.0-alpha1 Marks an issue as being completely or in parts released in 8.1.0-alpha1 version:8.1.0 Marks an issue as being completely or in parts released in 8.1.0

Comments

@npepinpe
Copy link
Member

Description

The Atomix log storage implementation is currently located in the log stream module. This means the logstream, and anything depending on it, are dependent on Atomix as well. This is unnecessary, and in fact wrong. We should move the implementation into the broker, which will simplify the dependency tree and decouple our Raft from the engine.
@npepinpe npepinpe added kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. team/distributed labels Mar 24, 2022
@pihme
Copy link
Contributor

pihme commented Apr 11, 2022

This may be a little trickier than anticipated. Mostly due to AtormixLogStorageRule, and the tests that depend on it.

Implementing an alternative log storage for these tests would be an option.Or these test could also be moved into broker 🤔

@npepinpe
Copy link
Member Author

Considering how simple the in memory storage implementation is, would we consider adding it there? After all, we ended up re-implementing it in eze, zeebe-process-test, and also in the engine itself for tests. We could rewrite the API tests in the log streams to use that implementation.

@pihme
Copy link
Contributor

pihme commented Apr 19, 2022

I think this is a valid option, yes.

@npepinpe npepinpe assigned npepinpe and unassigned npepinpe Apr 20, 2022
@npepinpe npepinpe added the area/maintainability Marks an issue as improving the maintainability of the project label Apr 20, 2022
@Zelldon Zelldon mentioned this issue Apr 26, 2022
Merged
10 tasks
@Zelldon Zelldon self-assigned this Apr 26, 2022
zeebe-bors-camunda bot added a commit that referenced this issue Apr 28, 2022
@zeebe-bors-camunda @Zelldon
merge: #9235
596cb07 
9235: Move atomix logstorage to broker r=Zelldon a=Zelldon

## Description

I wanted to code a bit on Zeebe again, so sorry if someone else wanted to do it.

* I moved the list logstorage to the logstreams to simplify the tests, as discussed in #8975 
* Engine tests uses the same logstorage
* Moved atomix logstorage related tests to broker
* Move atomix logstorage related classes to broker
* Remove atomix-cluster dependency from logstreams
* Removed some unused classes

Note: One thing I was not sure, is regarding the Listener. We seem to check for a certain atomix exception. I'm not sure whether this is really necessary. I guess we could also just fail here if this is anyway no leader anymore.

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

## Related issues

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

closes #8975 



Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
@deepthidevaki deepthidevaki added the version:8.1.0-alpha1 Marks an issue as being completely or in parts released in 8.1.0-alpha1 label May 3, 2022
@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
Assignees

@Zelldon Zelldon

Labels
area/maintainability Marks an issue as improving the maintainability of the project kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. version:8.1.0-alpha1 Marks an issue as being completely or in parts released in 8.1.0-alpha1 version:8.1.0 Marks an issue as being completely or in parts released in 8.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Move atomix logstorage to broker camunda/camunda
4 participants
@npepinpe @deepthidevaki @Zelldon @pihme