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.

Sign up for GitHub

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

Jump to bottom

[WFCORE-6601][WFCORE-6607][WFCORE-6608] ModuleSpecification dependency tracking fixes #5768

Merged
merged 3 commits into from
Nov 17, 2023
Merged

[WFCORE-6601][WFCORE-6607][WFCORE-6608] ModuleSpecification dependency tracking fixes #5768

merged 3 commits into from
Nov 17, 2023

Conversation

bstansberry
Copy link
Contributor

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Nov 15, 2023
yersan
yersan approved these changes Nov 16, 2023
View reviewed changes
Copy link
Collaborator

@yersan yersan left a comment

Choose a reason for hiding this comment

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

Added minor optional comments that, in my opinion, should not prevent merging it. This change has been confirmed that fix WFCORE-6606.

I also wondering

  1. Does it deserve a test case using EARs (it can be added later)?
  2. There is no follow up for the deprecation, I guess, we can get it done for the next WildFly release, but not sure what is our cadence regarding deprecations and removals.

server/src/main/java/org/jboss/as/server/deployment/module/ModuleSpecification.java Outdated Show resolved Hide resolved
server/src/main/java/org/jboss/as/server/deployment/module/ModuleSpecification.java Show resolved Hide resolved
@bstansberry
[WFLY-6608] Better synchronize the list and set views of ModuleSpecif…
8bafe46 
…ication dependencies

Deprecate the accessors for the list views.
Deprecate the direct access to the mutable user dependency set; replace with a remove method.
@bstansberry
Copy link
Contributor Author

@yersan and @scottmarlow

You've approved this but I just updated it a fair bit. The changes don't affect how the existing WF code interact with this though, so the good TCK results won't be affected.

  1. I added @deprecated annotations to a couple fields per Yeray's input.
  2. I cleaned up some other javadoc and IDE warning stuff.
  3. I added a test case.
  4. Most substantially I changed the new 'remove' method I'd added, from:
removeUserDependency(ModuleDependency)

to

removeUserDependencies(Predicate<ModuleDependency>)

That's a much better API to replace the intended use of getMutableUserDependencies(), which was to allow ApplicationClientDependencyProcessor to remove some dependencies that match its rules. Now it can pass in the rules and let ModuleSpecification deal with the internal state.

This last change shouldn't affect any existing behavior, as nothing calls this remove method yet (other than the new test).

@scottmarlow
Copy link
Contributor

@yersan and @scottmarlow

You've approved this but I just updated it a fair bit. The changes don't affect how the existing WF code interact with this though, so the good TCK results won't be affected.

1. I added @deprecated annotations to a couple fields per Yeray's input.

2. I cleaned up some other javadoc and IDE warning stuff.

3. I added a test case.

4. Most substantially I changed the new 'remove' method I'd added, from:

removeUserDependency(ModuleDependency)

to

removeUserDependencies(Predicate<ModuleDependency>)

That's a much better API to replace the intended use of getMutableUserDependencies(), which was to allow ApplicationClientDependencyProcessor to remove some dependencies that match its rules. Now it can pass in the rules and let ModuleSpecification deal with the internal state.

This last change shouldn't affect any existing behavior, as nothing calls this remove method yet (other than the new test).

I really like thisPredicate<ModuleDependency> change. I was thinking about the contract issue and this is a nice solution.

scottmarlow
scottmarlow reviewed Nov 16, 2023
View reviewed changes
server/src/main/java/org/jboss/as/server/deployment/module/ModuleSpecProcessor.java
module.addSystemDependencies(moduleSpecification.getSystemDependencies());
module.addLocalDependencies(moduleSpecification.getLocalDependencies());
for(ModuleDependency dep : moduleSpecification.getUserDependencies()) {
module.addSystemDependencies(moduleSpecification.getSystemDependenciesSet());
Copy link
Contributor

@scottmarlow scottmarlow Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't mention this before but I really like the reduction in (potential for apps that have duplicate module dependencies) memory use by using Set with these changes.

@yersan yersan added the ready-for-merge This PR is ready to be merged and fulfills all requirements label Nov 17, 2023
@yersan yersan merged commit 07a310e into wildfly:main Nov 17, 2023
@yersan
Copy link
Collaborator

yersan commented Nov 17, 2023

Thanks @bstansberry and @scottmarlow !

@bstansberry bstansberry deleted the WFCORE-6608 branch November 17, 2023 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottmarlow scottmarlow scottmarlow approved these changes

@yersan yersan yersan approved these changes

Assignees
No one assigned
Labels
deps-ok Dependencies have been checked, and there are no significant changes ready-for-merge This PR is ready to be merged and fulfills all requirements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bstansberry @scottmarlow @yersan