search

camunda / camunda-bpm-platform

Flexible framework for workflow and decision automation with BPMN and DMN. Integration with Quarkus, Spring, Spring Boot, CDI.
https://camunda.com/
Apache License 2.0
4.03k stars 1.53k forks source link

feat(engine): Populating process instance id in jobs #4334

Open punitdarira opened 1 month ago

punitdarira commented 1 month ago

related to #4205

punitdarira commented 1 month ago

@yanavasileva, The AcquirableJobEntity does not contain a processDefinitionId field. Should we add a new field, or is the processDefinitionId set elsewhere? Additionally, could you explain how the job logs will be visible in the Historic Process Definition page and Job Log view once the processDefinitionId is set?

yanavasileva commented 1 month ago

Hi @punitdarira,

Thank you for raising this pull request and showing interest in our product.

The AcquirableJobEntity does not contain a processDefinitionId field. Should we add a new field, or is the processDefinitionId set elsewhere?

Why you need the field there? The processDefinitionId field should be populated in a similar way to the JobEntity here: https://github.com/camunda/camunda-bpm-platform/pull/4334/files#diff-bbebf2d653092eaf76c21343a5d422db745c0f5ab55aeef084435be0e64ab11bR173-R175

Additionally, could you explain how the job logs will be visible in the Historic Process Definition page and Job Log view once the processDefinitionId is set?

In Cockpit, we display all job logs with the specific processDefinitionId. Once a job has set this id, the historic job log will inherit it, and out-of-the-box the entities will be populated in the mentioned page and view. No action is required for this comment.

Lastly, don't forget to cover the made changes with test cases.

Hope that helps you to continue with the contribution.

Best, Yana

punitdarira commented 1 month ago

Hi @punitdarira,

Thank you for raising this pull request and showing interest in our product.

The AcquirableJobEntity does not contain a processDefinitionId field. Should we add a new field, or is the processDefinitionId set elsewhere?

Why you need the field there? The processDefinitionId field should be populated in a similar way to the JobEntity here: https://github.com/camunda/camunda-bpm-platform/pull/4334/files#diff-bbebf2d653092eaf76c21343a5d422db745c0f5ab55aeef084435be0e64ab11bR173-R175

Additionally, could you explain how the job logs will be visible in the Historic Process Definition page and Job Log view once the processDefinitionId is set?

In Cockpit, we display all job logs with the specific processDefinitionId. Once a job has set this id, the historic job log will inherit it, and out-of-the-box the entities will be populated in the mentioned page and view. No action is required for this comment.

Lastly, don't forget to cover the made changes with test cases.

Hope that helps you to continue with the contribution.

Best, Yana

Hi Yana, The process definitionId is already being set to the job- https://github.com/camunda/camunda-bpm-platform/blob/3f6cb3055232c0fa9958e7a351af6ea0eafd7bb2/engine/src/main/java/org/camunda/bpm/engine/impl/jobexecutor/JobDeclaration.java#L93

The above line gets invoked from org.camunda.bpm.engine.impl.batch.AbstractBatchJobHandler.createBatchJob(BatchEntity batch, ByteArrayEntity configuration) Do we still need to set again in createJobEntities()?

Regarding tests, can you please guide whether we should go for new test file for AbstractBatchJobHandler where we simply mock or do we deploy a bpmn file in a test case and check whether instance and definition ids are set?

yanavasileva commented 1 month ago

The process definitionId is already being set to the job

I missed that, however, in debug mode and in general the historic job logs doesn't inherit the process definition id from the batch job entity. At the moment, I can't say why is that. So we need to understand why and fix it eventually.

Regarding tests, can you please guide

I think we can add a test case(s) to existing classes that test batch execution. Here's an example of single invocation test for set variables we have at the moment: https://github.com/camunda/camunda-bpm-platform/blob/master/engine/src/test/java/org/camunda/bpm/engine/test/api/runtime/SetVariablesBatchTest.java#L673-L695. examples of interesting batches to cover - migration, modification, set retries

punitdarira commented 1 month ago

JobDeclaration.java

Hi Yana, The property was being set as null as the value was being fetched from ACT_RU_JOBDEF and right now we are not setting the processdefinitionid in this table. I've added the code to get the value from batch configuration. I've added a new test case but the setup of test case is similar to RestartProcessInstanceAsyncTest.shouldSetExecutionStartTimeInBatchAndHistory so to avoid running the same setup twice, we can maybe just add a new assert statement to the above mentioned method with a comment.

punitdarira commented 2 weeks ago

Hi @yanavasileva, I could think of 3 solutions here.

  1. To make sure every applicable batch type adds the process definition id in ACT_RU_JOBDEF. So that by default it gets picked up via https://github.com/camunda/camunda-bpm-platform/blob/3f6cb3055232c0fa9958e7a351af6ea0eafd7bb2/engine/src/main/java/org/camunda/bpm/engine/impl/jobexecutor/JobDeclaration.java#L93
  2. Doing the same thing as point 1 but with configuration in ACT_RU_BATCH
  3. In AbstractBatchJobHandler.createJobEntities, looking up process definition id from HistoricProcessInstance table using instance id and setting it in the JobEntity.

3rd solution would be the least invasive and clearer. Can you please advice?

yanavasileva commented 1 week ago

@punitdarira, I am not sure either of the options is great when we think about performance. For example, at first glance option 3 (fetching the process definition id via the process instance) sounds good, but that implementation would mean for each batch execution job to have an additional query to the database. At the moment, I don't think it will be worth it. I might overlooked that aspect during the creation of the ticket. I will sync with the team then update you and the ticket with the outcome.

punitdarira commented 1 week ago

@punitdarira, I am not sure either of the options is great when we think about performance. For example, at first glance option 3 (fetching the process definition id via the process instance) sounds good, but that implementation would mean for each batch execution job to have an additional query to the database. At the moment, I don't think it will be worth it. I might overlooked that aspect during the creation of the ticket. I will sync with the team then update you and the ticket with the outcome.

sounds good

yanavasileva commented 1 week ago

@punitdarira, I checked with the team, we want to keep it simple for now and only populate the processDefinitionId field for restart batch operations. Decision: https://github.com/camunda/camunda-bpm-platform/issues/4205#issuecomment-2196882992.