chore(data-migrator): add retry loop on initial migration#1033
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the history migration flow to automatically retry skipped entities during an initial migration run (to resolve cross-entity-type dependency ordering issues), and refactors history migrators to be type-addressable via a common getType()/migrateByType(TYPE) API. It also introduces explicit skip handling for unsupported Camunda 7 standalone and CMMN history entities, with new/updated QA coverage.
Changes:
- Add an automatic
RETRY_SKIPPEDloop insideHistoryMigrator.migrate()and aggregate permanently-skipped entities for warning logs. - Refactor history migrators around
BaseMigrator.getType()andHistoryMigrator.migrateByType(TYPE); update tests/docs to use the new API. - Add unsupported-history skip reasons (standalone tasks, CMMN tasks/variables) and integration tests/resources validating behavior.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/TESTING_GUIDELINES.md | Updates testing guidance to use migrateByType(TYPE) calls. |
| data-migrator/qa/integration-tests/src/test/resources/io/camunda/migration/data/bpmn/c7/retry-process.bpmn | Adds a complex BPMN model used for retry-loop integration testing. |
| data-migrator/qa/integration-tests/src/test/java/io/camunda/migration/data/qa/persistence/DropSchemaTest.java | Switches tests to migrateByType for process instance migration. |
| data-migrator/qa/integration-tests/src/test/java/io/camunda/migration/data/qa/history/entity/variables/interceptor/HistoryVariableInterceptorTest.java | Aligns log assertions with updated skip logging behavior/message format. |
| data-migrator/qa/integration-tests/src/test/java/io/camunda/migration/data/qa/history/entity/variables/interceptor/HistoryDecisionInstanceInterceptorTest.java | Aligns log assertions with updated skip logging behavior/message format. |
| data-migrator/qa/integration-tests/src/test/java/io/camunda/migration/data/qa/history/entity/interceptor/HistoryPresetParentPropertiesTest.java | Replaces per-entity migrate methods with migrateByType. |
| data-migrator/qa/integration-tests/src/test/java/io/camunda/migration/data/qa/history/entity/HistoryProcessInstanceTest.java | Adjusts test flow to explicitly migrate prerequisites via migrateByType. |
| data-migrator/qa/integration-tests/src/test/java/io/camunda/migration/data/qa/history/entity/HistoryAuditLogUserTaskTest.java | Updates audit log test sequencing using migrateByType. |
| data-migrator/qa/integration-tests/src/test/java/io/camunda/migration/data/qa/history/entity/HistoryAuditLogTest.java | Uses migrateByType for audit log migration path. |
| data-migrator/qa/integration-tests/src/test/java/io/camunda/migration/data/qa/history/TransactionRollbackTest.java | Refactors rollback tests to call migrateByType and updated TYPE imports. |
| data-migrator/qa/integration-tests/src/test/java/io/camunda/migration/data/qa/history/HistoryMigrationSkippingTest.java | Updates skip scenario tests to new migration API and revised skip reasons. |
| data-migrator/qa/integration-tests/src/test/java/io/camunda/migration/data/qa/history/HistoryMigrationRetryTest.java | Updates retry tests to use migrateByType and new skip reason expectations. |
| data-migrator/qa/integration-tests/src/test/java/io/camunda/migration/data/qa/history/HistoryMigrationListSkippedTest.java | Updates list-skipped tests for new migration API and TYPE usage. |
| data-migrator/qa/integration-tests/src/test/java/io/camunda/migration/data/qa/history/HistoryMigrationListSkippedFilterTest.java | Updates list-skipped filtering tests to use migrateByType and TYPE constants. |
| data-migrator/qa/integration-tests/src/test/java/io/camunda/migration/data/qa/history/HistoryMigrationInitialRetryLoopTest.java | Adds an integration test validating the initial migration retry loop across dependent types. |
| data-migrator/qa/integration-tests/src/test/java/io/camunda/migration/data/qa/history/HistoryMigrationAbstractTest.java | Adds helper queries for “list all” user tasks/variables needed by new tests. |
| data-migrator/qa/integration-tests/src/test/java/io/camunda/migration/data/qa/history/HistoryCmmNotSupportedTest.java | Adds integration tests ensuring CMMN history tasks/variables are skipped with clear reasons. |
| data-migrator/core/src/main/java/io/camunda/migration/data/impl/logging/VariableServiceLogs.java | Renames interceptor logging helpers and changes interceptor log level usage. |
| data-migrator/core/src/main/java/io/camunda/migration/data/impl/logging/HistoryMigratorLogs.java | Adds new skip reasons; refactors skip logging to WARN/DEBUG variants. |
| data-migrator/core/src/main/java/io/camunda/migration/data/impl/history/migrator/VariableMigrator.java | Implements getType(), refactors fetch/retry wiring, and skips CMMN variables. |
| data-migrator/core/src/main/java/io/camunda/migration/data/impl/history/migrator/UserTaskMigrator.java | Implements getType(), refactors fetch/retry wiring, and skips standalone/CMMN user tasks. |
| data-migrator/core/src/main/java/io/camunda/migration/data/impl/history/migrator/ProcessInstanceMigrator.java | Implements getType() and refactors fetch/retry wiring. |
| data-migrator/core/src/main/java/io/camunda/migration/data/impl/history/migrator/ProcessDefinitionMigrator.java | Implements getType() and refactors fetch/retry wiring. |
| data-migrator/core/src/main/java/io/camunda/migration/data/impl/history/migrator/IncidentMigrator.java | Implements getType() and refactors fetch/retry wiring. |
| data-migrator/core/src/main/java/io/camunda/migration/data/impl/history/migrator/FormMigrator.java | Implements getType() and refactors fetch/retry wiring; uses getType() when checking migration. |
| data-migrator/core/src/main/java/io/camunda/migration/data/impl/history/migrator/FlowNodeMigrator.java | Implements getType() and refactors fetch/retry wiring. |
| data-migrator/core/src/main/java/io/camunda/migration/data/impl/history/migrator/DecisionRequirementsMigrator.java | Implements getType() and refactors fetch/retry wiring. |
| data-migrator/core/src/main/java/io/camunda/migration/data/impl/history/migrator/DecisionInstanceMigrator.java | Implements getType() and refactors fetch/retry wiring. |
| data-migrator/core/src/main/java/io/camunda/migration/data/impl/history/migrator/DecisionDefinitionMigrator.java | Implements getType() and refactors fetch/retry wiring. |
| data-migrator/core/src/main/java/io/camunda/migration/data/impl/history/migrator/BaseMigrator.java | Centralizes fetch/retry with getType(), returns skip exceptions in retry mode, and changes skip logging behavior. |
| data-migrator/core/src/main/java/io/camunda/migration/data/impl/history/migrator/AuditLogMigrator.java | Implements getType() and refactors fetch/retry wiring. |
| data-migrator/core/src/main/java/io/camunda/migration/data/impl/history/EntitySkippedException.java | Adds equality/hash semantics based on (C7 id, type) for deduplication. |
| data-migrator/core/src/main/java/io/camunda/migration/data/impl/clients/DbClient.java | Enhances skipped-entity pagination to optionally collect skip exceptions; provides overload for Consumer use. |
| data-migrator/core/src/main/java/io/camunda/migration/data/impl/VariableService.java | Switches interceptor failure logging helper + exception message builder. |
| data-migrator/core/src/main/java/io/camunda/migration/data/HistoryMigrator.java | Adds initial retry loop behavior, migrator registry, and migrateByType(TYPE) API. |
...-migrator/core/src/main/java/io/camunda/migration/data/impl/logging/VariableServiceLogs.java
Show resolved
Hide resolved
...igrator/core/src/main/java/io/camunda/migration/data/impl/history/migrator/BaseMigrator.java
Show resolved
Hide resolved
data-migrator/core/src/main/java/io/camunda/migration/data/impl/clients/DbClient.java
Outdated
Show resolved
Hide resolved
data-migrator/core/src/main/java/io/camunda/migration/data/HistoryMigrator.java
Show resolved
Hide resolved
0a376dd to
d25c705
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 53 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
data-migrator/core/src/main/java/io/camunda/migration/data/impl/history/migrator/HistoryEntityMigrator.java:167
retry()assumesfetchForRetryHandler().apply(c7Id)always returns a non-null C7 entity. If the entity was deleted/cleaned up in C7,c7Entitywill be null andgetC7Entity(null)will throw (seeC7Entity.of(Object)), aborting the retry loop. Handle the null case explicitly (e.g., mark as permanently skipped with a clear reason, or delete the mapping record) so retries don’t crash on missing source entities.
data-migrator/core/src/main/java/io/camunda/migration/data/impl/history/migrator/HistoryEntityMigrator.java:103- In
saveRecord(...)the RETRY_SKIPPED branch callsupdateC8KeyByC7IdAndType(..., null, ...), which doesn’t change anything and also never persists the (potentially updated)skipReason. This means--list-skippedcan show stale reasons after a retry skip. Consider updating the skip reason viadbClient.updateSkipReason(c7Id, type, skipReason)when retrying and an entity is still skipped (and avoid the no-op c8Key update).
data-migrator/distro/src/main/java/io/camunda/migration/data/app/MigratorApp.java
Show resolved
Hide resolved
...sts/src/test/java/io/camunda/migration/data/qa/runtime/SkipAndRetryProcessInstancesTest.java
Show resolved
Hide resolved
3a5a166 to
ddb4ee9
Compare
yanavasileva
left a comment
There was a problem hiding this comment.
👍 Looks good in general, list skipped didn't work for me.
...ion-tests/src/test/java/io/camunda/migration/data/qa/history/entity/HistoryFlowNodeTest.java
Show resolved
Hide resolved
...ion-tests/src/test/java/io/camunda/migration/data/qa/history/entity/HistoryFlowNodeTest.java
Outdated
Show resolved
Hide resolved
...ion-tests/src/test/java/io/camunda/migration/data/qa/history/entity/HistoryIncidentTest.java
Outdated
Show resolved
Hide resolved
...ts/src/test/java/io/camunda/migration/data/qa/history/entity/HistoryProcessInstanceTest.java
Outdated
Show resolved
Hide resolved
...ts/src/test/java/io/camunda/migration/data/qa/history/entity/HistoryProcessInstanceTest.java
Outdated
Show resolved
Hide resolved
...ts/src/test/java/io/camunda/migration/data/qa/history/entity/HistoryProcessInstanceTest.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/io/camunda/migration/data/impl/history/migrator/HistoryEntityMigrator.java
Show resolved
Hide resolved
data-migrator/core/src/main/java/io/camunda/migration/data/HistoryMigrator.java
Show resolved
Hide resolved
ddb4ee9 to
563cb63
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 53 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
data-migrator/core/src/main/java/io/camunda/migration/data/impl/history/migrator/HistoryEntityMigrator.java:102
- In RETRY_SKIPPED mode,
saveRecord()only callsupdateC8KeyByC7IdAndType(..., null, type)and never updates the skip reason. This drops the existing behavior where retrying a still-skipped entity can updateSKIP_REASON(there is a dedicatedDbClient.updateSkipReason(...)). Consider updating the skip reason whenskipReason != nullin RETRY_SKIPPED mode (and keep the existing reason when skipReason is null).
data-migrator/core/src/main/java/io/camunda/migration/data/HistoryMigrator.java
Show resolved
Hide resolved
| <bpmndi:BPMNDiagram id="BPMNDiagram_1"> | ||
| <bpmndi:BPMNPlane id="BPMNPlane_1" bpmnElement="Process_0oq8ck8"> | ||
| <bpmndi:BPMNShape id="StartEvent_1_di" bpmnElement="StartEvent_1"> |
There was a problem hiding this comment.
The BPMN DI plane references a non-existent process element (bpmnElement="Process_0oq8ck8"), while the actual process id is retry-process. This makes the DI section inconsistent and can break rendering/editing in BPMN tooling. Update the bpmnElement to point at the real process id (or remove the DI section if it’s intentionally minimal for tests).
...ion-tests/src/test/java/io/camunda/migration/data/qa/history/HistoryCmmNotSupportedTest.java
Show resolved
Hide resolved
|
The failing tests will be fixed after #1044 is merged. |
Removed pull request branch filters from CI workflow.
563cb63 to
c15e36a
Compare
yanavasileva
left a comment
There was a problem hiding this comment.
👍 Looks good. Thank you for considering my review hints.
Pull Request Template
Description
related to #1027
Type of Change
Testing Checklist
Black-Box Testing Requirements
DbClient,IdKeyMapper,..impl..packages except logging constants)ArchitectureTestvalidates these rules)Test Coverage
Architecture Compliance
Run architecture tests to ensure compliance:
mvn test -Dtest=ArchitectureTestIf architecture tests fail, refactor your tests to use:
LogCapturerfor log assertionscamundaClient.new*SearchRequest()for C8 queriesDocumentation
Checklist
Related Issues