-
Notifications
You must be signed in to change notification settings - Fork 638
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
Introduce cache for process versions #12475
Conversation
594bb6d
to
5fcea5d
Compare
In order to avoid confusion rename NextValueManager to ProcessVersionManager, since it is only purpose is to manage the version of a process.
In order to avoid unnecessary RocksDB access, we introduce an cache for the process version. The version is likely to not change too often, which will cause it to move to a low layer in RocksDB, which might make access slow in the hot path.
5fcea5d
to
743bec2
Compare
Just for posterity: Today we had a sync PR review with @berkaycanbc and @koevskinikola where I explained all the details and reasoning behind it. We had a good discussion where I also was able to answer some open questions. There was no blocker found during this call, so I expect we can go ahead to merge the PR after it is accepted by any of the reviewers. |
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.
LGTM!
bors r+ |
Build succeeded: |
Backport failed for Please cherry-pick the changes locally. git fetch origin stable/8.1
git worktree add -d .worktree/backport-12475-to-stable/8.1 origin/stable/8.1
cd .worktree/backport-12475-to-stable/8.1
git checkout -b backport-12475-to-stable/8.1
ancref=$(git merge-base 9dbedea61c4e96297ee9076c159d0902195d9257 743bec2ca5c74372c3d0d359106d06396c41b008)
git cherry-pick -x $ancref..743bec2ca5c74372c3d0d359106d06396c41b008 |
Successfully created backport PR for |
12496: [Backport stable/8.1] Introduce cache for process versions r=oleschoenburg a=megglos # Description Backport of #12475 to `stable/8.1`. Note: This doesn't contain the rename commit ec2f789 of the NextValueManager to ProcessVersionManager, as `NextValueManager` is used in other occasions on the 8.1 branch. relates to #12034 Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
Description
When executing a process instance we often have to get the process model and related version to it.
When running a cluster for a while (creating a lot of state etc.) the process version will eventually be migrated to a low level of RocksDB (potentially L3), because most of the time process models are not deployed that often. In other words, if a key is not updated it will be moved to a lower level by RocksDB. Accessing lower levels of RocksDB is slower than accessing higher levels or mem tables.
You might ask why is it slow, even if we repeatedly access via RocksDB, why is it not in the cache? There are multiple reasons for it.
In order to avoid running into issues with cold data, which is mostly static data, we can introduce our own caches, to work around this. This allows us to avoid unnecessary RocksDB access, unnecessary io, etc.
This PR does the following:
Performance:
We run again a JMH benchmark with the changes and can see that the performance slightly increased (potentially 8%), not significant but it will likely come into play with other changes later.
See more details here
Related issues
closes #12034
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
backport stable/1.3
) to the PR, in case that fails you need to create backports manually.Testing:
Documentation:
Other teams:
If the change impacts another team an issue has been created for this team, explaining what they need to do to support this change.
Please refer to our review guidelines.