Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix duplicate check element for instances and variables #285

Conversation

nitram509
Copy link
Collaborator

Based on the discussion in PR #280,
this is the actual bug fix for issue #278.
This PR depends on #284, because of the author's preference to split #280 into separate PRs.

This bug fix uses the approach of transient creation of an ID, based on PartitionID and Position.
Also, tests are added, to show how it works.
The duplicate check is implemented again.

@CLAassistant
Copy link

CLAassistant commented Aug 26, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@saig0 saig0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nitram509 thank you for the contribution 🎉

It looks good. Please rebase the PR on the latest changes and check my comments.

Comment on lines 49 to 55
return Schema.ProcessInstanceRecord.newBuilder()
.setElementId(elementId)
.setMetadata(Schema.RecordMetadata.newBuilder()
.setIntent(ProcessInstanceIntent.ELEMENT_ACTIVATED.name()))
.build();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could set the partition id and the position of the record. This would make the behavior more explicit. Like in the other test case.

import org.springframework.test.context.support.AnnotationConfigContextLoader;
import org.springframework.transaction.annotation.Transactional;

@RunWith(SpringJUnit4ClassRunner.class)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please migrate the tests to JUnit 5.

For example, replace @RunWith(SpringJUnit4ClassRunner.class) with @SpringBootTest.

nitram509 added a commit to nitram509/zeebe-simple-monitor that referenced this pull request Aug 27, 2021
@nitram509
Copy link
Collaborator Author

@saig0 it seems I messed up the rebase ... still working on this PR to get rid of the merge conflict.

This solution favors creating new ID, based on a composition of partitionId and position.
See comments in PR camunda-community-hub#280 and Issue camunda-community-hub#278 for more details.
Tests are added to show implementation works properly
Unfortunately, because of JPA requires an @id field, a new field was introduced
and there's an UPGRADE instruction documented, so people out there would be able to migrate their installations.
add partitionId and position as recommended, camunda-community-hub#285 (review)
@nitram509 nitram509 force-pushed the rework-duplicate-check-element-instances-and-variables branch from fb25b9d to f38ceda Compare August 27, 2021 21:33
@nitram509
Copy link
Collaborator Author

@saig0 OK, I fixed it. Sorry for any previous confusion, I did force push on my branch and now this PR has the latest changes from master included.
I'm glad to contribute :)

@saig0 saig0 self-requested a review August 30, 2021 03:24
Copy link
Contributor

@saig0 saig0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@saig0 saig0 merged commit 3123b08 into camunda-community-hub:master Aug 30, 2021
@saig0 saig0 changed the title bugfix for issue #278 - rework duplicate check element instances and variables Fix duplicate check element for instances and variables Aug 30, 2021
@nitram509 nitram509 deleted the rework-duplicate-check-element-instances-and-variables branch September 27, 2022 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants