-
Notifications
You must be signed in to change notification settings - Fork 590
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
Separate mutable and immutable states #6233
Conversation
* extract mutable and immutable interfaces of the states * flatten the hierarchy of states * move methods to the responsible states
fc79d07
to
cda90d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes make me very happy. The naming convention feels really good.
In the immutable variants I did have to think for some time about 2 methods that change internal state of the class, but not the actual state the class provides access to. Considering they don't actually change db state, its fine to keep them in the immutable interface.
I did find a method that should not be in an immutable interface (see comment) and I have a small naming suggestion.
Great work on this.
* @param eventScopeKey the key of the event scope | ||
* @return the next event trigger or null if none exist | ||
*/ | ||
EventTrigger pollEventTrigger(long eventScopeKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be in the mutable variant of this interface, since it removes the polled event trigger from state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch 👍
|
||
void putLatestVersionDigest(DirectBuffer processId, DirectBuffer digest); | ||
|
||
int getNextWorkflowVersion(String bpmnProcessId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename this to nextWorkflowVersion
to make it more clear that this is not a getter without a side-effect
@korthout thanks for the fast review. I applied your hints. Please have a look :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fast changes. LGTM 🥇
bors |
bors r+ |
Build succeeded: |
Description
io.zeebe.engine.state.mutable
io.zeebe.engine.state.immutable
Mutable*State
Db*State
- to indicate that they are based on theZeebeDB
ZeebeDb
give access to all statesHints for the Reviewer
Related issues
closes #6170
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
backport stable/0.25
) to the PR, in case that fails you need to create backports manually.Testing:
Documentation: