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

Don't write Job activated record #5991

Merged
merged 3 commits into from
Dec 10, 2020
Merged

Don't write Job activated record #5991

merged 3 commits into from
Dec 10, 2020

Conversation

Zelldon
Copy link
Member

@Zelldon Zelldon commented Dec 9, 2020

Description

  • The job activated record is no longer written to the log. It is already present in JobBatchRecord.
  • Job record is also not copied multiple times just wrapped without variables for state updates.
  • Fixed MetricExporter, since it relied on the Job.ACTIVATED event.
  • Fix several engine tests

I run a benchmark and saw no problems, only with the Job activation metrics, which I have fixed afterwards.

Base

General

general
general2

Resources

res

Latency

latency
latency2

No activated job event

General

no-general
no-general2

Resources

no-res

Latency

no-latency
no-latency2

I set up a new benchmark with the metrics fix.

Related issues

closes #2182

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 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

 * job activated record is no longer written to the log. It is already
   present in JobBatchRecord
 * Job record is also not copied multiple times just wrapped without
   variables for state updates
@Zelldon Zelldon requested a review from saig0 December 9, 2020 14:31
Copy link
Member

@saig0 saig0 left a comment

Choose a reason for hiding this comment

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

Nice. Thanks Chris 🍰

Just one minor comment regarding a test.
Please add a note to the release announcements because it is kind of a user-facing change.

@Zelldon Zelldon closed this Dec 10, 2020
@Zelldon Zelldon reopened this Dec 10, 2020
@Zelldon
Copy link
Member Author

Zelldon commented Dec 10, 2020

Ups github scrolled to slow then I mistakenly closed it sorry. Thanks for the review @saig0

Latency metrics are also working now:
latency2

I will squash your commit and then merge if you accept again 😅 😁

 Based on not writting job activated record tests are adjusted
@Zelldon
Copy link
Member Author

Zelldon commented Dec 10, 2020

bors r+

@zeebe-bors
Copy link
Contributor

zeebe-bors bot commented Dec 10, 2020

Build succeeded:

@zeebe-bors zeebe-bors bot merged commit d96032b into develop Dec 10, 2020
@zeebe-bors zeebe-bors bot deleted the zell-no-job-activation branch December 10, 2020 07:33
@npepinpe
Copy link
Member

Will this be flagged by the inconsistency detection? Could it break an update?

@Zelldon
Copy link
Member Author

Zelldon commented Dec 10, 2020

I think this will not be detected by our detection. I just tested it with 0.25.2 creating instances, activating jobs etc and then updating to my snapshot. No problems found. The thing is on the earlier version the event exist. The follow up event is tested, whether the source exist and has same key etc. But we will not detect whether we are still writing an event.

Maybe @saig0 can explain this better.

@saig0 saig0 mentioned this pull request Mar 22, 2021
9 tasks
ghost pushed a commit that referenced this pull request Mar 23, 2021
6603: Remove the job activated intent r=saig0 a=saig0

## Description

* the job activated intent is not used anymore
* remove the intent and adjust the ordinals

## Related issues

Follow-up of #5991

## 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](https://github.com/zeebe-io/zeebe/compare/stable/0.24...develop?expand=1&template=backport_template.md&title=[Backport%200.24]) 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](https://drive.google.com/drive/u/0/folders/1DTIeswnEEq-NggJ25rm2BsDjcCQpDape)


Co-authored-by: Philipp Ossler <philipp.ossler@gmail.com>
npepinpe added a commit that referenced this pull request Sep 9, 2021
Removes artificial batch size limitation when activating jobs. This was
a left over from when we used to also write a Job.ACTIVATED record,
which was removed with #5991, but we still kept the max batch length to
half of the record length.
ghost pushed a commit that referenced this pull request Sep 10, 2021
7795: Remove artificial job activation batch size limit r=npepinpe a=npepinpe

## Description

This PR removes the artificial limitation of the job batch record length to be half of the maximum message size. This was previously there because we used to write the activated job into the job batch once, and again as a separate record. This was removed in #5991, but we didn't remove the limit.

This PR makes it so that we can now activate single jobs whose payload is almost the max message size, minus some framing/header space, and also activate slightly more jobs in a single batch.

It's still a little counter intuitive since we need a little extra space, so you can't quite fit exactly the `maxMessageSize` as a variable, but that's anyway not possible to do in most other commands as well.

## Related issues

closes #6207 



Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
github-merge-queue bot pushed a commit that referenced this pull request Mar 14, 2024
…before fully constructed (#5991)

* fix(backend): fix race condition where DatabaseInfo bean is accessed before fully constructed

* fix integration tests that relied on static application context updates from other tests

* Change race condition fix for DatabaseInfo to use DependsOn instead of Autowire
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.

Do not write job activated record
3 participants