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

[WFCORE-6658] Convert ControlledProcessStateService to the current MS… #5824

Merged
merged 1 commit into from
Jan 19, 2024

Conversation

bstansberry
Copy link
Contributor

@bstansberry
Copy link
Contributor Author

@ropalka FYI.

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Jan 7, 2024
@wildfly wildfly deleted a comment from wildfly-ci Jan 9, 2024
@wildfly wildfly deleted a comment from wildfly-ci Jan 9, 2024
@bstansberry bstansberry added hold Do not merge this PR and removed hold Do not merge this PR labels Jan 10, 2024
Copy link
Collaborator

@yersan yersan left a comment

Choose a reason for hiding this comment

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

LGTM, @ropalka could you also take a quick look?

My very minor detail is that now the new SimpleService that provides the ProcessStateNotifier is not invoking consumer.accept(null) on the stop method as we did in ControlledProcessStateService:61, I don't think it could be an issue, but I am not certainly sure if that could cause any unexpected side effect. It is done when the service is being stopped, so it looks like safe.

@@ -221,6 +230,7 @@ private synchronized void establishModelControllerClient(ControlledProcessState.
if (state != ControlledProcessState.State.STOPPING && state != ControlledProcessState.State.STOPPED && serviceContainer != null) {
ModelControllerClientFactory clientFactory;
try {
// TODO replace this in start() with the ServiceActivator approach we used to capture the ProcessStateNotifier
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've just added WFCORE-6662 as a follow up to cover this.

@yersan
Copy link
Collaborator

yersan commented Jan 17, 2024

@ropalka FYI in case you have missed it.

Copy link
Contributor

@ropalka ropalka left a comment

Choose a reason for hiding this comment

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

LGTM

@yersan yersan added the ready-for-merge This PR is ready to be merged and fulfills all requirements label Jan 18, 2024
@yersan yersan merged commit 3ed7084 into wildfly:main Jan 19, 2024
12 checks passed
@yersan
Copy link
Collaborator

yersan commented Jan 19, 2024

Thanks @bstansberry and @ropalka

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-ok Dependencies have been checked, and there are no significant changes ready-for-merge This PR is ready to be merged and fulfills all requirements
Projects
None yet
3 participants