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.

Sign up for GitHub

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

Jump to bottom

Fix not calling shutdown on EmbeddedEventStore in combination with javax #2585

Merged
merged 2 commits into from
Feb 1, 2023
Merged

Fix not calling shutdown on EmbeddedEventStore in combination with javax #2585

merged 2 commits into from
Feb 1, 2023

Conversation

gklijs
Copy link
Member

@gklijs gklijs commented Feb 1, 2023

By moving the trigger for the shutdown of the EmbeddedEventStore from using an annotation, to implementing LifeCycle it's no longer javax or jakarta specific. This makes sure it always works, and removes the need for a legacy EmbeddedEventStore.

Fixes #2584

@gklijs gklijs added Type: Bug Use to signal issues that describe a bug within the system. Priority 1: Must Highest priority. A release cannot be made if this issue isn’t resolved. Status: In Progress Use to signal this issue is actively worked on. labels Feb 1, 2023
@gklijs gklijs added this to the Release 4.7.1 milestone Feb 1, 2023
@gklijs gklijs self-assigned this Feb 1, 2023
@gklijs gklijs requested a review from smcvb February 1, 2023 13:06
smcvb
smcvb requested changes Feb 1, 2023
View reviewed changes
Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

Two small pointers, but enough to request changes for, imo.

...g/src/main/java/org/axonframework/eventsourcing/eventstore/legacyjpa/EmbeddedEventStore.java Outdated Show resolved Hide resolved
messaging/src/main/java/org/axonframework/lifecycle/Phase.java Outdated Show resolved Hide resolved
@gklijs
By moving the trigger for the shutdown of the EmbeddedEventStore fr…
ea60951 
…om using an annotation, to implementing `LifeCycle` it's no longer `javax` or `jakarta` specific. This makes sure it always works.
@gklijs
Add test to verify shutdown is called.
0011b8e
smcvb
smcvb approved these changes Feb 1, 2023
View reviewed changes
Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

My concerns have been addressed, hence I'm approving this pull request.

@sonarcloud
Copy link

sonarcloud bot commented Feb 1, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug B 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@gklijs gklijs merged commit c157509 into axon-4.7.x Feb 1, 2023
@gklijs gklijs added Status: Resolved Use to signal that work on this issue is done. and removed Status: In Progress Use to signal this issue is actively worked on. labels Feb 1, 2023
@gklijs gklijs deleted the issues/2584 branch February 1, 2023 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smcvb smcvb smcvb approved these changes

Assignees

@gklijs gklijs

Labels
Priority 1: Must Highest priority. A release cannot be made if this issue isn’t resolved. Status: Resolved Use to signal that work on this issue is done. Type: Bug Use to signal issues that describe a bug within the system.
Projects
None yet
Milestone
Release 4.7.1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@gklijs @smcvb