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

[WFLY-18016] Refactoring - eliminating usage of deprecated CapabilityServiceBuilder.addCapabilityRequirement() methods #16840

Merged
merged 3 commits into from Jun 11, 2023
Merged

[WFLY-18016] Refactoring - eliminating usage of deprecated CapabilityServiceBuilder.addCapabilityRequirement() methods #16840

merged 3 commits into from Jun 11, 2023

Conversation

ropalka
Copy link
Contributor

@ropalka ropalka commented May 12, 2023

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label May 12, 2023
bstansberry
bstansberry reviewed May 13, 2023
View reviewed changes
...sources-agroal/src/main/java/org/wildfly/extension/datasources/agroal/DataSourceService.java Outdated
@@ -27,6 +27,7 @@
import io.agroal.api.transaction.TransactionIntegration;
import io.agroal.narayana.NarayanaTransactionIntegration;
import org.ietf.jgss.GSSException;
import org.jboss.as.server.deployment.DelegatingSupplier;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer not to use the server module as a utility lib.

Extensions can use the deployment package in DUPs, but this isn't a DUP. (At some point we should factor out that package into its own module.)

AIUI using this just eliminates the need for some null checks Optional.isPresent checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed @bstansberry . There was introduced a new DelegatingSupplier class in EE subsystem and AGROAL subsystem was rewritten to don't use DelegatingSupplier at all.
We're using DelegatingSupplier not because of Optional.isPresent() checks but because there are some places where supplier injection happens later than at the service installation time.

@ropalka ropalka force-pushed the deprecated-capabilities-cleanup branch from 5a657f6 to 4cbb095 Compare May 15, 2023 10:38
@ropalka ropalka force-pushed the deprecated-capabilities-cleanup branch from 4cbb095 to db58774 Compare June 2, 2023 10:29
bstansberry
bstansberry reviewed Jun 9, 2023
View reviewed changes
...rces-agroal/src/main/java/org/wildfly/extension/datasources/agroal/DataSourceOperations.java Outdated
import org.wildfly.security.credential.source.CredentialSource;

import java.util.function.Consumer;
import java.util.function.Supplier;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't change this unless there is some other change needed, but the imports here are out of order.

The jakarta.transaction one was already out of order due to my sed change from javax to jakarta, which took it out of the "java. and javax. before others" category.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

bstansberry
bstansberry reviewed Jun 9, 2023
View reviewed changes
...sources-agroal/src/main/java/org/wildfly/extension/datasources/agroal/DataSourceService.java Outdated
@@ -115,7 +126,7 @@ public void start(StartContext context) throws StartException {

if (jta || xa) {
TransactionManager transactionManager = ContextTransactionManager.getInstance();
TransactionSynchronizationRegistry transactionSynchronizationRegistry = transactionSynchronizationRegistryInjector.getValue();
TransactionSynchronizationRegistry transactionSynchronizationRegistry = transactionSynchronizationRegistrySupplier.get();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think transactionSynchronizationRegistrySupplier can be null if jta is false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct - thanks for spotting this out ! It is fixed now.

@ropalka
[WFLY-18016] Refactoring part of EE subsystem - eliminating usage of …
e66170a 
…deprecated CapabilityServiceBuilder.addCapabilityRequirement() methods
@ropalka
[WFLY-18016] Refactoring part of TXN subsystem - eliminating usage of…
61baae8 
… deprecated CapabilityServiceBuilder.addCapabilityRequirement() methods
@ropalka ropalka force-pushed the deprecated-capabilities-cleanup branch from db58774 to a02bbee Compare June 9, 2023 19:12
@ropalka
[WFLY-18016] Refactoring part of AGROAL subsystem - eliminating usage…
2feaab5 
… of deprecated CapabilityServiceBuilder.addCapabilityRequirement() methods
@ropalka ropalka force-pushed the deprecated-capabilities-cleanup branch from a02bbee to 2feaab5 Compare June 9, 2023 19:30
@bstansberry
Copy link
Contributor

/retest

@bstansberry bstansberry merged commit dc014ca into wildfly:main Jun 11, 2023
13 of 14 checks passed
@bstansberry
Copy link
Contributor

Thanks @ropalka

@ropalka ropalka deleted the deprecated-capabilities-cleanup branch June 13, 2023 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bstansberry bstansberry bstansberry approved these changes

Assignees
No one assigned
Labels
29.Alpha1 deps-ok Dependencies have been checked, and there are no significant changes
Projects
None yet
Milestone
No milestone
2 participants
@ropalka @bstansberry