Extract KeyGenerator from ZeebeState #10440
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Create the key generator in the StreamProcessor, since the key is part of the Record metadata, which is know by the StreamProcessor and needs to be reset during replay. The keygenerator is part of the RecordProcessorContext and corresponding API.
The keygenerator is no longer created inside the ZeebeState, it is passed into the CTOR from outside. Coming from the RecordProcessorContext, created by the streamprocessor.
18 tasks
Zelldon
force-pushed
the
zell-extract-keygenerator
branch
from
0267307
to
34bc13b
Compare
10 tasks
zeebe-bors-camunda bot
added a commit
that referenced
this pull request
Sep 26, 2022
10412: Add a standalone app that can be used to restore a broker from a backup r=deepthidevaki a=deepthidevaki ## Description * Added a new module for restore. * Adds a RestoreManager that coordinates restoring of all partitions. It first determines which partitions to restore from based on the given `BrokerCfg`. This configuration must be the same one used by the Broker when it is started after the restore. Because of this, restore module depens on broker module. But since it needs access to the configuration and `RaftPartitionGroupFactory`, there is no other way. There is scope of splitting broker to take these shared configs out of it. * Added a `RestoreApp` in dist. My plan was to add it in restore module. But it needs access to BrokerConfig, which is built in dist. So the app is added to dist module. Alternative is to copy components `BrokerConfiguration` and `WorkerDirectory` to restore module. * The backup id to restore from is passed as a command line argument `--backupId=x`. This is parsed by spring boot. I decided to keep it simple for now, since we are planning to release it as experimental feature anyway. If we decide to support this app long term, then I suggest to switch to something like picocli that we use in zdb. A basic test to verify a broker can be restored is added. More scenarios should be added later. PS:- App is not added to the distribution yet. ## Related issues related #10263 10440: Extract KeyGenerator from ZeebeState r=oleschoenburg a=Zelldon ## Description As preparation for the StreamProcessor - Engine module split we have to move out the Keygenerator out from the zeebeState, since this part of the stream processing (key is part of RecordMetadata). We have to reset the keys during replay and for that we need access to the key generator. The Key generator is now created in the StreamProcessor and passed into the ZeebeState (as dependency), this allowed to make minimal changes. At the start I removed completely the KEygenerator from the ZeebeState, but then we would need to adjust a LOT of setup code, which doesn't felt necessary tbh since it just about where this generator is created. The change also allowed us to remove the ZeebeState from the StreamProcessorContext 💪 `@npepinpe` `@saig0` who ever has time to review this. I think `@npepinpe` is currently a bit busy with other stuff related to backup and reviews, maybe you have time to review it `@saig0` <!-- Please explain the changes you made here. --> ## Related issues <!-- Which issues are closed by this PR or are related --> related to #10130 related to #9727 10483: deps(maven): bump checkstyle from 10.3.3 to 10.3.4 r=github-actions[bot] a=dependabot[bot] Bumps [checkstyle](https://github.com/checkstyle/checkstyle) from 10.3.3 to 10.3.4. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/checkstyle/checkstyle/releases">checkstyle's releases</a>.</em></p> <blockquote> <h2>checkstyle-10.3.4</h2> <p><a href="https://checkstyle.org/releasenotes.html#Release_10.3.4">https://checkstyle.org/releasenotes.html#Release_10.3.4</a></p> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/checkstyle/checkstyle/commit/6de3b9f1feb5c80450af2ad646f82ba34dc250bb"><code>6de3b9f</code></a> [maven-release-plugin] prepare release checkstyle-10.3.4</li> <li><a href="https://github.com/checkstyle/checkstyle/commit/a4497de578d5a72b386e7ca88676da190584eece"><code>a4497de</code></a> doc: release notes 10.3.4</li> <li><a href="https://github.com/checkstyle/checkstyle/commit/32e8d374e67e5ef5c19a688064bb2a77b5947651"><code>32e8d37</code></a> Issue <a href="https://github-redirect.dependabot.com/checkstyle/checkstyle/issues/12145">#12145</a>: corrected tokens so all are required</li> <li><a href="https://github.com/checkstyle/checkstyle/commit/96e3e05a526bca3f7a157ca929666530eb0b089f"><code>96e3e05</code></a> dependency: bump pitest-accelerator-junit5 from 1.0.1 to 1.0.2</li> <li><a href="https://github.com/checkstyle/checkstyle/commit/37842d2f291b138b2e9d44b33350add3f9562f34"><code>37842d2</code></a> Issue <a href="https://github-redirect.dependabot.com/checkstyle/checkstyle/issues/3955">#3955</a>: corrected tokens so all are required</li> <li><a href="https://github.com/checkstyle/checkstyle/commit/38ee347a49376e80858c969f742e6461dfe7d731"><code>38ee347</code></a> minor: remove unnecessary checkstyle versions to diff.groovy</li> <li><a href="https://github.com/checkstyle/checkstyle/commit/318770dc1343e9be90b8304c275d9adaf4ad8d45"><code>318770d</code></a> dependency: bump slf4j-simple from 2.0.1 to 2.0.2</li> <li><a href="https://github.com/checkstyle/checkstyle/commit/119453600ff9f6accc247f748df8c0dd6c65a2af"><code>1194536</code></a> dependency: bump junit.version from 5.9.0 to 5.9.1</li> <li><a href="https://github.com/checkstyle/checkstyle/commit/5a56c16882b68e0aaedf3a2a29737f412bbd66be"><code>5a56c16</code></a> Issue <a href="https://github-redirect.dependabot.com/checkstyle/checkstyle/issues/12132">#12132</a>: Fix ArrayIndexOutOfBoundsException in pitest-survival-check-xml...</li> <li><a href="https://github.com/checkstyle/checkstyle/commit/701bd65dec286ab75fed35b99a580fc77c8ea03f"><code>701bd65</code></a> Issue <a href="https://github-redirect.dependabot.com/checkstyle/checkstyle/issues/12210">#12210</a>: Add method to ignore unstable checker framework violations</li> <li>Additional commits viewable in <a href="https://github.com/checkstyle/checkstyle/compare/checkstyle-10.3.3...checkstyle-10.3.4">compare view</a></li> </ul> </details> <br /> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting ``@dependabot` rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - ``@dependabot` rebase` will rebase this PR - ``@dependabot` recreate` will recreate this PR, overwriting any edits that have been made to it - ``@dependabot` merge` will merge this PR after your CI passes on it - ``@dependabot` squash and merge` will squash and merge this PR after your CI passes on it - ``@dependabot` cancel merge` will cancel a previously requested merge and block automerging - ``@dependabot` reopen` will reopen this PR if it is closed - ``@dependabot` close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - ``@dependabot` ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - ``@dependabot` ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - ``@dependabot` ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com> Co-authored-by: Christopher Zell <zelldon91@googlemail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
|
This PR was included in a batch that was canceled, it will be automatically retried |
saig0
approved these changes
Sep 26, 2022
Comment on lines
+68
to
+70
| // we don't need a key generator here, so we set it to null | ||
| state = | ||
| new ZeebeDbState(Protocol.DEPLOYMENT_PARTITION, zeebeDb, zeebeDb.createContext(), null); |
There was a problem hiding this comment.
🔧 We could prevent NPE by providing a null object for the key generator.
For example, implementing the key generator interface but returning -1 as the key or throwing an exception.
Currently, it doesn't hurt to pass null as the argument but in general, it could be considered as a code smell.
|
Build succeeded: |
10 tasks
zeebe-bors-camunda bot
added a commit
that referenced
this pull request
Sep 27, 2022
10491: Do not use null as KeyGenerator r=saig0 a=Zelldon ## Description Based on review hint here #10440 (comment) <!-- 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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
As preparation for the StreamProcessor - Engine module split we have to move out the Keygenerator out from the zeebeState, since this part of the stream processing (key is part of RecordMetadata). We have to reset the keys during replay and for that we need access to the key generator.
The Key generator is now created in the StreamProcessor and passed into the ZeebeState (as dependency), this allowed to make minimal changes. At the start I removed completely the KEygenerator from the ZeebeState, but then we would need to adjust a LOT of setup code, which doesn't felt necessary tbh since it just about where this generator is created.
The change also allowed us to remove the ZeebeState from the StreamProcessorContext 💪
@npepinpe @saig0 who ever has time to review this. I think @npepinpe is currently a bit busy with other stuff related to backup and reviews, maybe you have time to review it @saig0
Related issues
related to #10130
related to #9727
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:
Please refer to our review guidelines.