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

Restore the key generator on replay #6285

Merged
merged 1 commit into from Feb 11, 2021
Merged

Conversation

saig0
Copy link
Member

@saig0 saig0 commented Feb 9, 2021

Description

  • reset the key generator after the replay to the highest key that was written on the stream
  • update the key generator after skipping a command on replay
    • the key generator is used by not yet migrated processors
    • this part can be removed after all processors are migrated
  • extract an interface from the ZeebeState to avoid that internal controls (key generator, last processed state) are exposed

Hints for the reviewer:

  • I recommend to review the commits separately, the first includes the actual logic, the second is just the refactoring of the state
  • the test StreamProcessorReplayTest#shouldRestoreKeyGeneratorAfterSkippingCommand() will be removed with the migration logic after all processors are migrated

Related issues

closes #6164

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/0.25) 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.)
  • New content is added to the release announcement

@saig0 saig0 requested a review from korthout February 9, 2021 05:08
@saig0 saig0 marked this pull request as ready for review February 9, 2021 05:09
@saig0
Copy link
Member Author

saig0 commented Feb 10, 2021

Update: I added a third commit to ignore records from other partitions when restoring the key generator on replay. This happens when distributing a deployment to other partitions.

Copy link
Member

@korthout korthout left a comment

Choose a reason for hiding this comment

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

Nice and simple fix in the third commit. I generally like the changes, and everything seems correct. However, it's relative complex wording in an already complex context, so I've made some suggestions around understandability.

@saig0
Copy link
Member Author

saig0 commented Feb 11, 2021

@korthout I applied your hints. Please have a second look :)

@saig0 saig0 requested a review from korthout February 11, 2021 10:14
Copy link
Member

@korthout korthout left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. LGTM 🧸

* reset the key generator after replay to the highest key that was written on the stream
* update the key generator after skipping a command on replay. the key generator is used by not yet migrated processors
* ignore records from other partitions when restoring the key generator on replay
* extract an interface from the ZeebeState to avoid that internal controls (key generator, last processed state) are exposed
@saig0
Copy link
Member Author

saig0 commented Feb 11, 2021

bors r+

@zeebe-bors
Copy link
Contributor

zeebe-bors bot commented Feb 11, 2021

Build succeeded:

@zeebe-bors zeebe-bors bot merged commit 5b78ef4 into develop Feb 11, 2021
@zeebe-bors zeebe-bors bot deleted the 6164-replay-key-generator branch February 11, 2021 13:42
@npepinpe
Copy link
Member

It looks like this caused a performance drop of ~25 WI/s - do we have any ideas why?

@saig0
Copy link
Member Author

saig0 commented Feb 19, 2021

No 🤔
The changes should affect only the reprocessing/replay. But nothing has changed for the processing phase.

@npepinpe
Copy link
Member

Forgot to update, this PR didn't cause the changes - we found that the changes which caused a dip occurred in the following:

Quoting from Slack:

so i run several benchmarks, and what i see is slight dips over time it seems.

given that we have these merge commits in chronological order: 359aec0, 2ac8a25, f0f91fd, d73ae31

359aec0 does ~116 WI/s
2ac8a25 does ~116 WI/s
f0f91fd does ~105 WI/s
d73ae31 does ~95 WI/s

Between 2ac8a25 and f0f91fd we have:

* | | |   f0f91fd6d  Merge #6319 zeebe-bors[bot] Fri Feb 12 13:29:42 2021 +0000 
|\ \ \ \  
| |_|/ /  
|/| | |   
| * | | 27de076fb  chore(qa/it): fix flaky test Christopher Zell Thu Feb 11 13:28:43 2021 +0100 
* | | |   875dcf072  Merge #6289 zeebe-bors[bot] Fri Feb 12 12:04:57 2021 +0000 
|\ \ \ \  
| * | | | 4df43422d  chore(engine): add WorkflowRecord related tests Christopher Zell Thu Feb 11 08:54:46 2021 +0100 
| * | | | c622a9a94  chore(engine): write WorkflowRecord Christopher Zell Wed Feb 10 07:07:47 2021 +0100 
| * | | | 441b6b6f1  chore(protocol-impl): rename Workflow to WorkflowRecord Christopher Zell Wed Feb 10 06:38:52 2021 +0100 
| |/ / /  
* | | |   2ac8a2570  Merge #6328 zeebe-bors[bot] Fri Feb 12 10:09:02 2021 +0000

Between f0f91fd and d73ae31 we have:

* | | | |   d73ae31ea  Merge #6282 zeebe-bors[bot] Fri Feb 12 18:45:06 2021 +0000 
|\ \ \ \ \  
| |_|/ / /  
|/| | | |   
| * | | | 00ea30be7  chore(engine): apply review hints Nico Korthout Fri Feb 12 19:44:25 2021 +0100 
| * | | | f51a15f8d  refactor(engine): extract writers from processingcontext Nico Korthout Fri Feb 12 19:44:08 2021 +0100 
| * | | | 6a0f948a4  refactor(engine): remove appendNewEvent method Nico Korthout Fri Feb 12 19:42:53 2021 +0100 
| * | | | 96b1b060f  chore(engine): add StateWriter to processor init Nico Korthout Fri Feb 12 19:42:19 2021 +0100 
|/ / / /  
* | | |   f0f91fd6d  Merge #6319 zeebe-bors[bot] Fri Feb 12 13:29:42 2021 +0000

@npepinpe
Copy link
Member

npepinpe commented Feb 19, 2021

As of now I don't think it's dramatic enough to look into - we expect that in our current intermediate state things may be a little slower - but we should definitely keep an eye out when we run our weekly load tests, and probably take a bit of time at the end to profile/optimize.

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.

The stream processor restores the key generator on replay
3 participants