feat(engine): Populating process instance id in jobs

[punitdarira]

related to #4205

[punitdarira]

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

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]

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-

camunda-bpm-platform/engine/src/main/java/org/camunda/bpm/engine/impl/jobexecutor/JobDeclaration.java

Line 93 in 3f6cb30

job.setProcessDefinitionId(jobDefinition.getProcessDefinitionId());

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]

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

[yanavasileva]

As discussed above, the pull request needs more work.

[punitdarira]

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.