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

Update deployment descriptors for Concurrency 3.1 #27148

Conversation

KyleAure
Copy link
Member

@KyleAure KyleAure commented Dec 11, 2023

Updates our deployment descriptors for new values added for Concurrency 3.1 from the following sources:

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.

@LibbyBot
Copy link

Code analysis and actions

DO NOT DELETE THIS COMMENT.
  • 21 product code files were changed.

  • Please describe in a separate comment how you tested your changes.

  • 1 messages files were changed and need an L2 review.

  • @OpenLiberty/message-reviewer Please review.

  • dev/io.openliberty.concurrent.internal/resources/io/openliberty/concurrent/internal/resources/CWWKCMessages.nlsprops

  • 1 NLS files were changed and need an ID review.

  • @OpenLiberty/message-reviewer Please review.

  • dev/io.openliberty.concurrent.internal/resources/io/openliberty/concurrent/internal/resources/CWWKCMessages.nlsprops

@KyleAure KyleAure force-pushed the 27098-concurrent-3.1-qualifer-and-virutal branch 4 times, most recently from c90e336 to a6e93fb Compare December 11, 2023 22:19
Copy link

@jantley-ibm jantley-ibm left a 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.

Copy link
Contributor

@tbitonti tbitonti left a 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.

@KyleAure
Copy link
Member Author

@njr-11 @tbitonti @jantley-ibm Updated based off your feedback.

@KyleAure KyleAure force-pushed the 27098-concurrent-3.1-qualifer-and-virutal branch from 5c9c5b9 to 41f8834 Compare December 12, 2023 21:57
Copy link
Contributor

@njr-11 njr-11 left a 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);
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

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)

@posmith
Copy link

posmith commented Dec 18, 2023

Support message review completed.

@KyleAure KyleAure force-pushed the 27098-concurrent-3.1-qualifer-and-virutal branch from 8549a8e to b181523 Compare December 19, 2023 15:47
@KyleAure
Copy link
Member Author

Force pushed to resolve merge conflicts.

Comment on lines +248 to +249
//TODO is there a certain classloader I should be using to load this class? ApplicationClassLoader?
clazzArray[i] = Class.forName(classList[i]);
Copy link
Contributor

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.

KyleAure added a commit to KyleAure/open-liberty that referenced this pull request Jan 11, 2024
@KyleAure KyleAure deleted the 27098-concurrent-3.1-qualifer-and-virutal branch January 12, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants