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

[WFLY-14546] Restore naming context #14101

Merged
merged 1 commit into from Apr 28, 2021
Merged

Conversation

spyrkob
Copy link
Contributor

@spyrkob spyrkob commented Mar 10, 2021

@spyrkob spyrkob marked this pull request as draft March 10, 2021 11:29
@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Mar 10, 2021

@Override
public void execute(Runnable command) {
WeldTaskWrapper task = new WeldTaskWrapper(command, NamespaceContextSelector.getCurrentSelector());
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid wrapping if Weld isn't yet started, org.jboss.weld.Container#instance() can be used to check state.
You can see how we do similar check here - https://github.com/wildfly/wildfly/blob/master/weld/subsystem/src/main/java/org/jboss/as/weld/WeldProvider.java#L69-L70

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @manovotn, I added the check

@spyrkob spyrkob marked this pull request as ready for review March 17, 2021 08:01
@spyrkob
Copy link
Contributor Author

spyrkob commented Apr 1, 2021

@manovotn can you re-review this please? Should I ask anybody else for the review as well?

@manovotn
Copy link
Contributor

manovotn commented Apr 1, 2021

I am fine with this. It shouldn't affect bootstrap at least.
So the only question that stands is how this affects some event-heavy application because it is effectively an extra thread local set/unset for every event regardless of whether you use it or not. Might be interesting to see the comparison but I don't have such perf test at hand.

The only other person you can try to get review from is @mkouba ;-)

}
}

class WeldTaskWrapper implements Runnable {
Copy link
Contributor

Choose a reason for hiding this comment

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

This class and WeldExecutor do not seem to need to access the enclosing class (WeldExecutorServices) so it might make sense to make it static.

Copy link
Contributor

Choose a reason for hiding this comment

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

@spyrkob Can you make this change? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I missed that. It's updated now.

@mkouba
Copy link
Contributor

mkouba commented Apr 6, 2021

Looks good. But I would be interested in the numbers as well ;-).

@bstansberry
Copy link
Contributor

/retest

@bstansberry bstansberry merged commit b289439 into wildfly:master Apr 28, 2021
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
Projects
None yet
4 participants