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

PAYARA-3964 Allow configuration of MDB bean pool size with ActivationConfigProperty #4084

Merged
merged 2 commits into from
Aug 19, 2019

Conversation

smillidge
Copy link
Contributor

Added support for singleton-bean-pool and bean-pool MDB deployment descriptor elements as ActivationConfigProperty values in MDB annotations

…eployment descriptor elements as ActivationConfirProperty values
@smillidge smillidge added this to the 5.193 milestone Jul 10, 2019
Mini change - added space
@arjantijms
Copy link
Contributor

Jenkins test please

@arjantijms arjantijms changed the title PAYARA-3964 PAYARA-3964 Allow configuration of MDB bean pool size with ActivationConfigProperty Jul 11, 2019
@@ -114,7 +117,7 @@ protected EjbDescriptor createEjbDescriptor(String elementName,
* Set Annotation information to Descriptor.
* This method will also be invoked for an existing descriptor with
* annotation as user may not specific a complete xml.
* @param ejbDesc
* @param ejbDescMaxWaitTimeInMillis
Copy link
Member

Choose a reason for hiding this comment

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

?
The param name is ejbDesc

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, it's also not used for "MaxWaitTimeInMillis", so doesn't seem like a matter of forgetting to rename the method argument as well.

@@ -142,6 +145,26 @@ protected HandlerProcessingResult setEjbDescriptorInfo(
// xml override
if (acProp.propertyName().equals("resourceAdapter")) {
ejbMsgBeanDesc.setResourceAdapterMid(envProp.getValue());
} else if (acProp.propertyName().equals("MaxPoolSize")) {
Copy link
Member

Choose a reason for hiding this comment

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

Would a switch not be better?

@Pandrex247
Copy link
Member

Nothing immediately wrong from a quick glance, but a couple of quibbles

@@ -142,6 +145,26 @@ protected HandlerProcessingResult setEjbDescriptorInfo(
// xml override
if (acProp.propertyName().equals("resourceAdapter")) {
ejbMsgBeanDesc.setResourceAdapterMid(envProp.getValue());
} else if (acProp.propertyName().equals("MaxPoolSize")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The default value for these properties is defined in appserver/deployment/dol/src/main/java/com/sun/enterprise/deployment/DescriptorConstants.java.
String constants should also be added for these properties name.

Copy link
Contributor

@MattGill98 MattGill98 left a comment

Choose a reason for hiding this comment

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

Aside from the changes requested by others, LGTM and it seems to work fine too!

@Pandrex247
Copy link
Member

Review comments PR: smillidge#4

@MarkWareham MarkWareham merged commit 9f3f371 into payara:master Aug 19, 2019
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.

6 participants