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.

Sign up for GitHub

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

Jump to bottom

CAM-14006: avoid job log query on incident creation #1665

Closed
wants to merge 2 commits into from
Closed

CAM-14006: avoid job log query on incident creation #1665

wants to merge 2 commits into from

Conversation

ThorbenLindhauer
Copy link
Member

@ThorbenLindhauer ThorbenLindhauer commented Nov 15, 2021 •
edited
Loading

related to CAM-14006

@ThorbenLindhauer ThorbenLindhauer self-assigned this Nov 15, 2021
@ThorbenLindhauer ThorbenLindhauer marked this pull request as ready for review November 17, 2021 13:20
@ThorbenLindhauer
Copy link
Member Author

I'll squash the commits after the review to avoid retriggering the CI for now.

tmetzke
tmetzke previously approved these changes Nov 18, 2021
View reviewed changes
Copy link
Member

@tmetzke tmetzke left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code looks good to me 👍
Great that we have a migration test as well 👍
There is one import we can probably remove from one class, but that's it.

Edit: do we need to handle the same issue for the ExternalTaskEntity#getFailureLogId?

engine/src/main/java/org/camunda/bpm/engine/impl/persistence/entity/JobEntity.java Show resolved Hide resolved
@tmetzke tmetzke dismissed their stale review November 18, 2021 09:18

Forgot something

@ThorbenLindhauer
fix(engine): avoid job log query when creating an incident
3fa00f1 
- the job log reference is needed as the incident's history
  configuration
- the query can be expensive; so instead of trying to find the matching
  job log later, we carry the reference forward in the job and then
  to the incident when it is created

related to CAM-14006
@ThorbenLindhauer
fix(engine): avoid external task log query on incident creation
1741ca4 
- makes this behavior consistent with how it is implemented for jobs

related to CAM-14006
@ThorbenLindhauer
Copy link
Member Author

I have extended the solution to external tasks and removed the unnecessary import. Please re-review.

tmetzke
tmetzke requested changes Nov 22, 2021
View reviewed changes
Copy link
Member

@tmetzke tmetzke left a comment

Choose a reason for hiding this comment

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

Looks all good with regards to the External Task extension. 👍
We need to fix the CockroachDB upgrade script, however.
Feel free to merge after that.

engine/src/main/resources/org/camunda/bpm/engine/db/upgrade/cockroachdb_engine_7.16_to_7.17.sql Show resolved Hide resolved
@ThorbenLindhauer
Copy link
Member Author

Merged via 0c54d4f and c42c645

@ThorbenLindhauer ThorbenLindhauer deleted the CAM-14006 branch November 22, 2021 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmetzke tmetzke tmetzke requested changes

Assignees

@ThorbenLindhauer ThorbenLindhauer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ThorbenLindhauer @tmetzke