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

punitdarira · 2024-05-14T03:45:18Z

related to #4205

punitdarira · 2024-05-14T04:10:50Z

@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 · 2024-05-16T18:30:49Z

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 · 2024-05-20T06:52:51Z

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 · 2024-05-24T13:21:11Z

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.

related to camunda#4205

punitdarira · 2024-06-02T03:01:20Z

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.

yanavasileva

The proposed changes cause failures in other batch tests. Also, I would expect the have tests for all batches for which this change make sense.

we can maybe just add a new assert statement to the above mentioned method with a comment

Yes, you can consider doing that where it make sense add comments (and/or adjust test name) so people are not lost what is tested.

engine/src/main/java/org/camunda/bpm/engine/impl/batch/AbstractBatchJobHandler.java

yanavasileva · 2024-06-12T13:11:50Z

...e/src/test/java/org/camunda/bpm/engine/test/api/runtime/RestartProcessInstanceAsyncTest.java

+    assertEquals(processInstance.getProcessDefinitionId(), jobLog.getProcessDefinitionId());
+    assertEquals(processInstance.getRootProcessInstanceId(), jobLog.getProcessInstanceId());


❌ The checks are performed for the historic job log, I would expect to check the jobs as well.

punitdarira · 2024-06-20T07:02:09Z

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

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

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

Line 93 in 3f6cb30

job.setProcessDefinitionId(jobDefinition.getProcessDefinitionId());
Doing the same thing as point 1 but with configuration in ACT_RU_BATCH
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 · 2024-06-26T16:06:03Z

@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 · 2024-06-26T16:54:44Z

@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 · 2024-06-28T13:17:58Z

@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: #4205 (comment).

yanavasileva

Added adjustment for the processDefinitionId field.

engine/src/main/java/org/camunda/bpm/engine/impl/batch/AbstractBatchJobHandler.java

related to camunda#4205

...e/src/test/java/org/camunda/bpm/engine/test/api/runtime/RestartProcessInstanceAsyncTest.java

related to camunda#4205

yanavasileva · 2024-08-26T12:37:55Z

@punitdarira, are you willing to add further test coverage that the process instance id is populated for other/all type of batch operations?

yanavasileva · 2024-08-26T12:40:27Z

engine/src/main/java/org/camunda/bpm/engine/impl/RestartProcessInstancesJobHandler.java

+import org.camunda.bpm.engine.impl.util.JsonUtil;
+
+import static org.camunda.bpm.engine.impl.RestartProcessInstancesBatchConfigurationJsonConverter.PROCESS_DEFINITION_ID;


❌ Not needed anymore:

Suggested change

import org.camunda.bpm.engine.impl.util.JsonUtil;

import static org.camunda.bpm.engine.impl.RestartProcessInstancesBatchConfigurationJsonConverter.PROCESS_DEFINITION_ID;

punitdarira · 2024-08-28T01:01:36Z

@punitdarira, are you willing to add further test coverage that the process instance id is populated for other/all type of batch operations?

@yanavasileva
Sure Yana

related to camunda#4334

related to camunda#4205

punitdarira · 2024-09-02T19:32:07Z

@yanavasileva ,
Added tests for below batches-
BatchSetVariables
DecisionSetRemovalTime
DeleteHistoricDecisionInstances
DeleteHistoricProcessInstances
DeleteProcessInstancesJob
MessageCorrelationBatchJob
MigrationBatchJob
ModificationBatchJob
ProcessSetRemovalTimeJob
RestartProcessInstanceBatchJob
UpdateProcessInstancesSuspendStateJob

However, there were 3 batch types which were operating on batches directly like-
BatchSetRemovalTime
SetExternalTaskRetriesJob
SetJobRetriesJob

For these jobs, the processInstanceId is not being set as expected. I can see the job's id being set instead of the processInstaceId. Just to confirm, the execution jobs for these 3 batches should have the process instacancesId of the transitive instance right?

yanavasileva · 2024-09-04T18:11:06Z

Thanks for adding the tests. 💯

For these jobs, the processInstanceId is not being set as expected. I can see the job's id being set instead of the processInstaceId. Just to confirm, the execution jobs for these 3 batches should have the process instacancesId of the transitive instance right?

I will check and get back to you. The first one, we won't need a process instance id but I am not sure for the others.

related to #4334

yanavasileva

I am not ready with the review. However, I am sharing my comments to prevent losing them.
Some adjustments needs to be done for suspend and migration batches, I will look at that.

yanavasileva · 2024-09-05T16:14:19Z

.../java/org/camunda/bpm/engine/test/api/history/BatchHistoricDecisionInstanceDeletionTest.java

+  @Test
+  public void shouldCreateProcessInstanceRelatedBatchJobsForSingleInvocations(){
+    // when
+    Batch batch = historyService.deleteHistoricDecisionInstancesAsync(decisionInstanceIds, null);
+    helper.executeSeedJob(batch);
+
+    //then
+    //Making sure that processInstanceId is set in execution jobs #4205
+    assertThat(helper.getExecutionJobs(batch))
+            .extracting("processInstanceId")
+            .containsExactlyInAnyOrder(decisionInstanceIds.toArray());
+  }
+


❌ The processInstanceId should not be populated for this type of a batch. It is incorrect to store the decision ids as process instance ids.

Suggested change

@Test

public void shouldCreateProcessInstanceRelatedBatchJobsForSingleInvocations(){

// when

Batch batch = historyService.deleteHistoricDecisionInstancesAsync(decisionInstanceIds, null);

helper.executeSeedJob(batch);

//then

//Making sure that processInstanceId is set in execution jobs #4205

assertThat(helper.getExecutionJobs(batch))

.extracting("processInstanceId")

.containsExactlyInAnyOrder(decisionInstanceIds.toArray());

}

yanavasileva · 2024-09-05T16:19:35Z

...src/test/java/org/camunda/bpm/engine/test/api/history/HistoryServiceAsyncOperationsTest.java

@@ -109,6 +111,21 @@ public void testDeleteHistoryProcessInstancesAsyncWithList() throws Exception {
    assertAllHistoricProcessInstancesAreDeleted();
  }

+  @Test
+  public void testDeleteHistoryProcessInstances_shouldCreateProcessInstanceRelatedBatchJobsForSingleInvocations() {


🔧 Better

Suggested change

public void testDeleteHistoryProcessInstances_shouldCreateProcessInstanceRelatedBatchJobsForSingleInvocations() {

public void shouldPopulateProcessInstanceIdToBatchJobsForSingleInvocations() {

yanavasileva · 2024-09-05T16:23:40Z

.../java/org/camunda/bpm/engine/test/api/history/removaltime/batch/BatchSetRemovalTimeTest.java

+
+    //Making sure that processInstanceId is set in execution jobs #4205
+    assertThat(executionJobs)
+            .extracting("processInstanceId")
+            .containsExactlyInAnyOrder(query.list().stream().map(HistoricDecisionInstance::getId).toArray());


❌ Process instance id should not be populated for decision related batches:

Suggested change

//Making sure that processInstanceId is set in execution jobs #4205

assertThat(executionJobs)

.extracting("processInstanceId")

.containsExactlyInAnyOrder(query.list().stream().map(HistoricDecisionInstance::getId).toArray());

yanavasileva · 2024-09-05T16:23:53Z

.../java/org/camunda/bpm/engine/test/api/history/removaltime/batch/BatchSetRemovalTimeTest.java

+
+    HistoricDecisionInstanceQuery query = historyService.createHistoricDecisionInstanceQuery();
+


❌ Not needed:

Suggested change

HistoricDecisionInstanceQuery query = historyService.createHistoricDecisionInstanceQuery();

yanavasileva · 2024-09-05T16:33:49Z

...ne/src/test/java/org/camunda/bpm/engine/test/api/runtime/ModificationExecutionAsyncTest.java

❌ ✍️ Please check test #createModificationJobs() is failing. processInstanceId assertions must be adjusted.

yanavasileva · 2024-09-05T16:40:19Z

...src/test/java/org/camunda/bpm/engine/test/api/runtime/RuntimeServiceAsyncOperationsTest.java

+
+

Suggested change

yanavasileva · 2024-09-05T17:51:49Z

...ava/org/camunda/bpm/engine/test/api/runtime/UpdateProcessInstancesSuspendStateAsyncTest.java

Note to myself:
❓ Tests failures due to Entity was updated by another transaction concurrently. What's wrong

See: org.camunda.bpm.engine.impl.persistence.entity.JobManager.updateJobSuspensionStateByProcessInstanceId(String, SuspensionState)

yanavasileva · 2024-09-05T18:32:29Z

...rc/test/java/org/camunda/bpm/engine/test/api/runtime/migration/batch/BatchMigrationTest.java

Note to myself:
Tests failures due to

java.lang.NullPointerException: null at org.camunda.bpm.engine.impl.persistence.entity.JobEntity.setExecution(JobEntity.java:259) at org.camunda.bpm.engine.impl.persistence.entity.ExecutionEntity.restoreProcessInstance(ExecutionEntity.java:1384)

See org.camunda.bpm.engine.impl.migration.instance.parser.MigratingInstanceParser.fetchJobs(CommandContext, String)

yanavasileva · 2024-09-05T18:47:10Z

engine/src/main/java/org/camunda/bpm/engine/impl/batch/AbstractBatchJobHandler.java

@@ -169,6 +169,11 @@ protected void createJobEntities(BatchEntity batch, T configuration, String depl
      ByteArrayEntity configurationEntity = saveConfiguration(byteArrayManager, jobConfiguration);

      JobEntity job = createBatchJob(batch, configurationEntity);
+
+      if (jobConfiguration.getIds() != null && jobConfiguration.getIds().size() == 1) {


Something like this will be required to avoid populating the ids for entities that are not process instance:

Suggested change

if (jobConfiguration.getIds() != null && jobConfiguration.getIds().size() == 1) {

if (jobConfiguration.getIds() != null && jobConfiguration.getIds().size() == 1

&& !(this instanceof DecisionSetRemovalTimeJobHandler)

&& !(this instanceof DeleteHistoricDecisionInstancesJobHandler)

&& !(this instanceof SetJobRetriesJobHandler)

&& !(this instanceof SetExternalTaskRetriesJobHandler)

// && !(this instanceof BatchSetRemovalTimeJobHandler)

) {

yanavasileva · 2024-09-26T16:27:28Z

Just dropping by to say that I didn't have the chance to work on this.
We already started with the other tasks related to this topic so I will provide feedback in the next weeks.

feat(engine): Populating process instance id in jobs

4e97dc1

related to camunda#4205

punitdarira marked this pull request as ready for review May 14, 2024 04:06

yanavasileva assigned punitdarira May 16, 2024

yanavasileva self-requested a review May 16, 2024 12:25

yanavasileva mentioned this pull request May 16, 2024

Populate processInstanceId for execution jobs when invocationsPerBatchJob=1 #4205

Open

2 tasks

yanavasileva requested changes May 24, 2024

View reviewed changes

feat(engine): Setting process definition ID in job log

af36f9b

related to camunda#4205

punitdarira requested a review from yanavasileva June 2, 2024 06:19

yanavasileva requested changes Jun 12, 2024

View reviewed changes

yanavasileva added the group:requested-info DRI: Yana label Jun 12, 2024

github-actions bot added the group:stale DRI: Yana label Jun 13, 2024

yanavasileva mentioned this pull request Jun 13, 2024

fix(workflows): fix yml reference #4430

Merged

punitdarira mentioned this pull request Jun 20, 2024

4214 restarted process instance #4420

Merged

github-actions bot added group:stale DRI: Yana and removed group:stale DRI: Yana labels Jun 21, 2024

yanavasileva removed group:requested-info DRI: Yana group:stale DRI: Yana labels Jun 26, 2024

yanavasileva requested changes Jun 28, 2024

View reviewed changes

engine/src/main/java/org/camunda/bpm/engine/impl/batch/AbstractBatchJobHandler.java Outdated Show resolved Hide resolved

engine/src/main/java/org/camunda/bpm/engine/impl/batch/AbstractBatchJobHandler.java Outdated Show resolved Hide resolved

yanavasileva added the group:requested-info DRI: Yana label Jun 28, 2024

github-actions bot added the group:stale DRI: Yana label Jun 29, 2024

fix(engine): Moved setting of processDefinitionId to RestartJobHandler

bdde986

related to camunda#4205

fix(engine): Remove sysout

100fa5b

related to camunda#4205

yanavasileva removed group:requested-info DRI: Yana group:stale DRI: Yana labels Jun 29, 2024

yanavasileva reviewed Jul 8, 2024

View reviewed changes

...e/src/test/java/org/camunda/bpm/engine/test/api/runtime/RestartProcessInstanceAsyncTest.java Show resolved Hide resolved

feat(engine): Assert for instance and definitionid in job

bd1c38d

related to camunda#4205

yanavasileva reviewed Aug 26, 2024

View reviewed changes

punitdarira added 3 commits September 1, 2024 22:32

feat(engine): Tests

c374253

related to camunda#4334

feat(engine): Tests

3af25b9

related to camunda#4205

feat(engine): Tests

d211061

related to camunda#4205

yanavasileva pushed a commit that referenced this pull request Sep 5, 2024

feat(engine): Tests

2da3d57

related to #4334

yanavasileva reviewed Sep 5, 2024

View reviewed changes

yanavasileva self-requested a review September 10, 2024 12:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

punitdarira commented May 14, 2024

punitdarira commented May 14, 2024

yanavasileva commented May 16, 2024

punitdarira commented May 20, 2024

yanavasileva commented May 24, 2024

yanavasileva left a comment

punitdarira commented Jun 2, 2024

yanavasileva left a comment

yanavasileva Jun 12, 2024

punitdarira commented Jun 20, 2024

yanavasileva commented Jun 26, 2024 •

edited

Loading

punitdarira commented Jun 26, 2024

yanavasileva commented Jun 28, 2024

yanavasileva left a comment

yanavasileva commented Aug 26, 2024

yanavasileva Aug 26, 2024

punitdarira commented Aug 28, 2024

punitdarira commented Sep 2, 2024

yanavasileva commented Sep 4, 2024

yanavasileva left a comment

yanavasileva Sep 5, 2024

yanavasileva Sep 5, 2024

yanavasileva Sep 5, 2024

yanavasileva Sep 5, 2024

yanavasileva Sep 5, 2024

yanavasileva Sep 5, 2024

yanavasileva Sep 5, 2024

yanavasileva Sep 5, 2024

yanavasileva Sep 5, 2024

yanavasileva Sep 5, 2024

yanavasileva commented Sep 26, 2024

		assertEquals(processInstance.getProcessDefinitionId(), jobLog.getProcessDefinitionId());
		assertEquals(processInstance.getRootProcessInstanceId(), jobLog.getProcessInstanceId());

		import org.camunda.bpm.engine.impl.util.JsonUtil;

		import static org.camunda.bpm.engine.impl.RestartProcessInstancesBatchConfigurationJsonConverter.PROCESS_DEFINITION_ID;

	public void testDeleteHistoryProcessInstances_shouldCreateProcessInstanceRelatedBatchJobsForSingleInvocations() {
	public void shouldPopulateProcessInstanceIdToBatchJobsForSingleInvocations() {


		HistoricDecisionInstanceQuery query = historyService.createHistoricDecisionInstanceQuery();

-      if (jobConfiguration.getIds() != null && jobConfiguration.getIds().size() == 1) {
+      if (jobConfiguration.getIds() != null && jobConfiguration.getIds().size() == 1
+          && !(this instanceof DecisionSetRemovalTimeJobHandler)
+          && !(this instanceof DeleteHistoricDecisionInstancesJobHandler)
+          && !(this instanceof SetJobRetriesJobHandler)
+          && !(this instanceof SetExternalTaskRetriesJobHandler)
+//          && !(this instanceof BatchSetRemovalTimeJobHandler)
+          ) {

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

Are you sure you want to change the base?

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

Conversation

punitdarira commented May 14, 2024

punitdarira commented May 14, 2024

yanavasileva commented May 16, 2024

punitdarira commented May 20, 2024

yanavasileva commented May 24, 2024

yanavasileva left a comment

Choose a reason for hiding this comment

punitdarira commented Jun 2, 2024

yanavasileva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

punitdarira commented Jun 20, 2024

yanavasileva commented Jun 26, 2024 • edited Loading

punitdarira commented Jun 26, 2024

yanavasileva commented Jun 28, 2024

yanavasileva left a comment

Choose a reason for hiding this comment

yanavasileva commented Aug 26, 2024

Choose a reason for hiding this comment

punitdarira commented Aug 28, 2024

punitdarira commented Sep 2, 2024

yanavasileva commented Sep 4, 2024

yanavasileva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanavasileva commented Sep 26, 2024

yanavasileva commented Jun 26, 2024 •

edited

Loading