-
Notifications
You must be signed in to change notification settings - Fork 592
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
Update deployment descriptors for Concurrency 3.1 #27148
Update deployment descriptors for Concurrency 3.1 #27148
Conversation
Code analysis and actionsDO NOT DELETE THIS COMMENT.
|
c90e336
to
a6e93fb
Compare
dev/com.ibm.ws.javaee.dd.common/src/com/ibm/ws/javaee/dd/common/ManagedExecutor.java
Outdated
Show resolved
Hide resolved
dev/com.ibm.ws.javaee.dd.common/src/com/ibm/ws/javaee/dd/common/ManagedScheduledExecutor.java
Outdated
Show resolved
Hide resolved
dev/com.ibm.ws.javaee.dd.common/src/com/ibm/ws/javaee/dd/common/ManagedThreadFactory.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ID review comments attached. Let me know if you have questions or concerns about suggested changes. When we have agreed on changes, I will add the "ID reviewed" label.
...rrent.internal/resources/io/openliberty/concurrent/internal/resources/CWWKCMessages.nlsprops
Outdated
Show resolved
Hide resolved
...rrent.internal/resources/io/openliberty/concurrent/internal/resources/CWWKCMessages.nlsprops
Outdated
Show resolved
Hide resolved
...rrent.internal/resources/io/openliberty/concurrent/internal/resources/CWWKCMessages.nlsprops
Outdated
Show resolved
Hide resolved
dev/com.ibm.ws.javaee.ddmodel/test/com/ibm/ws/javaee/ddmodel/app/AppTest.java
Outdated
Show resolved
Hide resolved
dev/com.ibm.ws.javaee.ddmodel/src/com/ibm/ws/javaee/ddmodel/app/ApplicationDDParser.java
Outdated
Show resolved
Hide resolved
dev/com.ibm.ws.javaee.ddmodel/src/com/ibm/ws/javaee/ddmodel/web/WebAppDDParser.java
Outdated
Show resolved
Hide resolved
dev/com.ibm.ws.javaee.dd.common/src/com/ibm/ws/javaee/dd/common/ContextService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the parser changes. I did not look at the runtime updates. I see a few very minor updates (missing copyright, a possible name change). The function looks good.
@njr-11 @tbitonti @jantley-ibm Updated based off your feedback. |
5c9c5b9
to
41f8834
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment about merging. I'm fine if you defer this to a separate pull.
@@ -110,7 +110,7 @@ public void merge(ContextServiceDefinition annotation, Class<?> instanceClass, M | |||
|
|||
unchanged = mergeAnnotationValue(unchanged, XMLunchanged, annotation.unchanged(), KEY_UNCHANGED, DEFAULT_UNCHANGED); | |||
|
|||
qualifier = mergeAnnotationValue(qualifier, XMLqualifer, annotation.qualifiers(), KEY_QUALIFIER, DEFAULT_QUALIFIER); | |||
qualifiers = mergeAnnotationValue(qualifiers, XMLqualifers, annotation.qualifiers(), KEY_QUALIFIERS, DEFAULT_QUALIFIERS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will behave correctly for a more complex type like Class[]
.
mergeAnnotationValue in InjectionBinding does newValue.equals(oldValue)
which will be an instance comparison for arrays. It looks like there is a special variant, mergeAnnotationProperties, to handle merging for the properties, and it is even more complex in that it appears to combine individual properties from one list into the other rather than just overwriting. Should the qualifiers behave similarly where the merge is a combination of the annotation values and <qualifier>
elements? Or should the set of the latter replace the former?
This same comment will apply to all 4 resource types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't done any builds on this yet and still waiting on reviews. I think I'll tackle this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@njr-11 Resolved this by adding an extra check when we merge XML values under this commit b181523
(#27148)
Support message review completed. |
8549a8e
to
b181523
Compare
Force pushed to resolve merge conflicts. |
dev/com.ibm.ws.javaee.ddmodel/src/com/ibm/ws/javaee/ddmodel/common/XSDTokenType.java
Outdated
Show resolved
Hide resolved
dev/com.ibm.ws.javaee.ddmodel/test/com/ibm/ws/javaee/ddmodel/DDJakarta11Elements.java
Outdated
Show resolved
Hide resolved
...rrent.internal/resources/io/openliberty/concurrent/internal/resources/CWWKCMessages.nlsprops
Outdated
Show resolved
Hide resolved
//TODO is there a certain classloader I should be using to load this class? ApplicationClassLoader? | ||
clazzArray[i] = Class.forName(classList[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need to be careful with making it a requirement to load classes if we aren't certain we can have the right class loader when doing it. There might be different class loaders between modules and for the application. If we don't have access to the correct class loader here, another option would be to perform the merge as String class names, to be loaded later. I'm not sure what is available. You'll need to work with Tom on this part.
Updates our deployment descriptors for new values added for Concurrency 3.1 from the following sources:
virtual=true
annotation atrribute (hint to request virtual threads jakartaee/concurrency#349)qualifiers
annotation attribute (concurrency resources via @Inject and with qualifiers jakartaee/concurrency#348)I think I'm most of the way there just have some questions tagged with //TODO that need some further review.
Would appreciate some feedback @tbitonti.