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-9954] Fixing too many services and dependencies created for EJBs issue #12215

Merged
merged 1 commit into from Apr 14, 2019

Conversation

@ropalka
Copy link
Contributor

ropalka commented Apr 5, 2019

https://issues.jboss.org/browse/WFLY-9954

Moving component related JNDI dependencies from o.j.a.s.d.Attachments.JNDI_DEPENDENCIES
to o.j.a.s.d.Attachments.COMPONENT_JNDI_DEPENDENCIES attachment.
Before this fix JNDI_DEPENDENCY_SERVICE had too many dependencies.
With this fix applied JNDI_DEPENDENCY_SERVICE will now depend on
JNDI_AGGREGATION_SERVICES and these aggregation services
will reference COMPONENT_JNDI_DEPENDENCIES that were referenced
by JNDI_DEPENDENCY_SERVICE before. In other words we are decreasing
JNDI_DEPENDENCY_SERVICE dependencies count and reorganizing (simplifying)
its dependency tree.

Copy link
Contributor

bstansberry left a comment

Just restating what this does, in case I'm wrong: This adds a service per component but by doing so reduces the number of dependencies another service has by replacing 5 dependencies with 1 dependency.

@@ -29,8 +29,11 @@
import org.jboss.as.server.deployment.DeploymentUnitProcessingException;
import org.jboss.as.server.deployment.DeploymentUnitProcessor;

import java.util.HashMap;

This comment has been minimized.

Copy link
@bstansberry

bstansberry Apr 9, 2019

Contributor

The standard import sorting has this before org.x

This comment has been minimized.

Copy link
@ropalka

ropalka Apr 9, 2019

Author Contributor

Fixed

@@ -85,7 +90,9 @@ private void bindServices(DeploymentUnit deploymentUnit, ServiceTarget serviceTa
serviceTarget.addService(inAppClientServiceName, inAppClientContainerService)
.addDependency(contextServiceName, ServiceBasedNamingStore.class, inAppClientContainerService.getNamingStoreInjector())
.install();
deploymentUnit.addToAttachmentList(org.jboss.as.server.deployment.Attachments.JNDI_DEPENDENCIES, inAppClientServiceName);
final Map<ServiceName, Set<ServiceName>> jndiComponentDependencies = deploymentUnit.getAttachment(org.jboss.as.server.deployment.Attachments.COMPONENT_JNDI_DEPENDENCIES);
final Set<ServiceName> jndiDependencies = jndiComponentDependencies.computeIfAbsent(contextServiceName, k-> new HashSet<>());

This comment has been minimized.

Copy link
@bstansberry

bstansberry Apr 9, 2019

Contributor

Using a lambda like this instead a few lines to do a get and if null, a put, adds to the amount of metaspace the server needs.

This comment has been minimized.

Copy link
@ropalka

ropalka Apr 9, 2019

Author Contributor

Fixed

from o.j.a.s.d.Attachments.JNDI_DEPENDENCIES
 to  o.j.a.s.d.Attachments.COMPONENT_JNDI_DEPENDENCIES attachment.
Before this fix JNDI_DEPENDENCY_SERVICE had too many dependencies.
With this fix applied JNDI_DEPENDENCY_SERVICE will now depend on
JNDI_AGGREGATION_SERVICES and these aggregation services
will reference COMPONENT_JNDI_DEPENDENCIES that were referenced
by JNDI_DEPENDENCY_SERVICE before. In other words we are decreasing
JNDI_DEPENDENCY_SERVICE dependencies count and reorganizing (simplifying)
its dependency tree.
@ropalka ropalka force-pushed the ropalka:WFLY-9954 branch from 4bf2acf to 960c736 Apr 9, 2019
@ropalka

This comment has been minimized.

Copy link
Contributor Author

ropalka commented Apr 9, 2019

Correct @bstansberry but we are replacing not 5 but just 4 dependencies with one intermediary aggregating dependency. This will allow us to deploy more than 7000 EJBs per deployment since now on.

Copy link
Contributor

bstansberry left a comment

I count 5; two in TransactionJndiBindingProcessor. ;)

@ropalka

This comment has been minimized.

Copy link
Contributor Author

ropalka commented Apr 10, 2019

You're of course right @bstansberry . I shouldn't even make elementary counting if I'm tired :)

@bstansberry bstansberry merged commit c3c150e into wildfly:master Apr 14, 2019
6 checks passed
6 checks passed
Linux - JDK 11 (Pull Request) - merge TeamCity build finished
Details
Linux - JDK 8 Finished TeamCity Build WildFly / Pull Request / Linux - JDK 8 : Tests passed: 4852, ignored: 134
Details
Linux - elytron - JDK 8 (Pull Request) - merge TeamCity build finished
Details
Linux with security manager - JDK 8 Finished TeamCity Build WildFly / Pull Request / Linux SM - JDK 8 : Tests passed: 4508, ignored: 152
Details
Windows - JDK 11 Finished TeamCity Build WildFly / Pull Request / Windows - JDK 11 : Tests passed: 4843, ignored: 141
Details
Windows - JDK 8 Finished TeamCity Build WildFly / Pull Request / Windows - JDK 8 : Tests passed: 4845, ignored: 139
Details
@ropalka ropalka deleted the ropalka:WFLY-9954 branch Apr 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.