chore(history): populate process instance treepath#1029
chore(history): populate process instance treepath#1029HeleneW-dot wants to merge 2 commits intomainfrom
Conversation
5154c57 to
062db2a
Compare
| .historyCleanupDate(c8HistoryCleanupDate) | ||
| .endDate(c8EndTime); | ||
| .endDate(c8EndTime) | ||
| .treePath(generateTreepath(builder.build().rootProcessInstanceKey(), processInstanceKey)); |
There was a problem hiding this comment.
To avoid calling build(), can be also the patch can be generated after Line 106. If there's no rootProcessInstanceKey, users can use interception to populate the treePath. We do it in other places as well.
There was a problem hiding this comment.
I agree that calling build is not required here.
Consider tracking rootProcessInstanceKey in a local variable instead. The variable is already set in the block above (lines 100-108). Extracting it would be cleaner
venetrius
left a comment
There was a problem hiding this comment.
See my comments.
If treePath needs to contain the full hierarchy (all intermediate PI keys for deep structures like A→B→C), the current generateTreepath approach won't work. It only encodes root and current node.
Could you confirm whether deeper hierarchies are in scope?
| public static String generateTreepath(Long rootProcessInstanceKey, Long processInstanceKey) { | ||
| return Objects.equals(rootProcessInstanceKey, processInstanceKey) ? | ||
| "PI_" + processInstanceKey : | ||
| "PI_" + rootProcessInstanceKey + "/PI_" + processInstanceKey; |
There was a problem hiding this comment.
❌ If the hierarchy has more then 2 levels for example a chain A→B→C, process C's treePath would be PI_A/PI_C, skipping B.
There was a problem hiding this comment.
See the ticket description/slack link, the treepath is truncated to only keep root and leaf
| .historyCleanupDate(c8HistoryCleanupDate) | ||
| .endDate(c8EndTime); | ||
| .endDate(c8EndTime) | ||
| .treePath(generateTreepath(builder.build().rootProcessInstanceKey(), processInstanceKey)); |
There was a problem hiding this comment.
I agree that calling build is not required here.
Consider tracking rootProcessInstanceKey in a local variable instead. The variable is already set in the block above (lines 100-108). Extracting it would be cleaner
...e/src/main/java/io/camunda/migration/data/impl/history/migrator/ProcessInstanceMigrator.java
Outdated
Show resolved
Hide resolved
...ts/src/test/java/io/camunda/migration/data/qa/history/entity/HistoryProcessInstanceTest.java
Outdated
Show resolved
Hide resolved
We only keep the truncated treepath with root and leaf, I documented this in the ticket together with the slack link for the discussion around this question |
88bb1cf to
6dd9fd5
Compare
6dd9fd5 to
8d69be7
Compare
related to #431
Pull Request Template
Description
Adds treepath migration for process instances.
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