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

Adds support for injecting UniversalConnectionPool instances #8378

Merged
merged 7 commits into from
Feb 15, 2024

Conversation

ljnelson
Copy link
Member

@ljnelson ljnelson commented Feb 13, 2024

Overview

This PR provides support for injecting UniversalConnectionPool instances, as well as the sole UniversalConnectionPoolManager instance.

To accomplish this it refactors the existing AbstractDataSourceExtension class to extend from a new-but-behaviorally-equivalent type-agnostic superclass, AbstractConfigurableExtension, then authors a new UniversalConnectionPoolExtension that also extends from it.

Making use of UniversalConnectionPool and UniversalConnectionPoolManager instances is something that only advanced use cases will do.

Motivation

For advanced use cases that require access to a UniversalConnectionPool instance that underlies a PoolDataSource, acquiring one is not straightforward. We should provide the ability to inject one, just as we provide the ability already to inject its related PoolDataSource.

Installation

To install the facilities enabled by this PR, an end user should ensure that the io.helidon.integrations.cdi:helidon-integrations-cdi-datasource-ucp artifact is on her runtime classpath. This artifact already exists and this PR simply adds features to it. Existing users of our Universal Connection Pool integration will have to take no additional action.

Maven Coordinates

<dependency>
  <groupId>io.helidon.integrations.cdi</groupId>
  <artifactId>helidon-integrations-cdi-datasource-ucp</artifactId>
  <scope>runtime</scope>
</dependency>

Usage

To use the facilities enabled by this PR, first an end user should ensure that there is configuration in place to create a Universal Connection Pool-managed data source, as described in our documentation. (The documentation there shows the creation of a Universal Connection Pool-managed data source named test.)

Next, she should use normal CDI facilities to inject this data source, its corresponding UniversalConnectionPool, or both:

import javax.sql.DataSource;
import jakarta.inject.Inject;
import jakarta.inject.Named;
import oracle.ucp.UniversalConnectionPool;
import oracle.ucp.admin.UniversalConnectionPoolManager;
import oracle.ucp.jdbc.PoolDataSource;

// Injects a PoolDataSource as an implementation of a DataSource, qualified with the
// "test" name, as exists currently in Helidon 4.0.5:
@Inject
@Named("test")
private javax.sql.DataSource ds; // assert ds instanceof PoolDataSource;

// Injects the exact same object:
@Inject
@Named("test)
private PoolDataSource pds; // assert pds.getConnectionPoolName().equals("test");

// NEW: Injects the backing UniversalConnectionPool, qualified with the "test" name
// (a feature for advanced use cases this PR enables):
@Inject
@Named("test")
private UniversalConnectionPool ucp; // assert ucp.getName().equals("test");

// Inject the sole UniversalConnectionPoolManager (a feature for advanced use cases
// this PR enables):
@Inject
private UniversalConnectionPoolManager ucpm;

Backwards Compatibility Notes

This PR maintains the existing signatures in AbstractDataSourceExtension, so no impact to our existing code or users' existing code is expected.

Documentation Impact

This supports advanced, esoteric use cases only, so we mayor may not want to document it explicitly, as ordinary users should not be interacting with the underlying pool directly, but, rather, working with PoolDataSource instances, which the existing UCPBackedDataSourceExtension class already provides support for. I worry that too much documentation here will lead to users trying to create already-existing data sources again by mistakenly manipulating the pool. This is a side effect of the way that the Universal Connection Pool APIs are designed.

@ljnelson ljnelson added enhancement New feature or request MP P4 cdi CDI jpa/jta integration java Pull requests that update Java code 4.x Version 4.x labels Feb 13, 2024
@ljnelson ljnelson self-assigned this Feb 13, 2024
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Feb 13, 2024
bom/pom.xml Outdated Show resolved Hide resolved
@tjquinno
Copy link
Member

I understand the words but maybe not the intent describing the contract among getPropertyPatternMatcher and the getName and getPropertyName methods.

The JavaDoc states that the latter two methods expect the caller to pass a Matcher that was previously returned from getPropertyPatternMatcher and which therefore was created specifically for the name provided to getPropertyPatternMatcher.

But if there are multiple properties for a given extension (IIUC there could be quite a few), then the developer's code will have multiple Matcher instances to keep track of and will need to make sure to pass the correct Matcher instance to getName or getPropertyName at any point in the code.

I realize these are protected methods, and so are likely to be accessible only to someone implementing a configured extension. We can probably expect a good degree of care from such a developer. Even so, with the existing approach the developer needs to get two things right: create and save the Matcher by passing the correct config property name and then later pass the correct Matcher to the other two methods to get the name or prop name.

Could we expose getName(String configPropertyName) and getPropertyName(String configPropertyName) and our abstract superclass would create the corresponding Matcher lazily and then cache and reuse it internally. That way the developer writing the code that invokes these methods needs to get only one thing right: the name of the config property passed to the methods.

Or alternatively. is there a role for another type--say, PropertyManager or some such--which the extension returns with the Matcher inside. The developer then invokes getName() and getPropertyName() on the PropertyManager. The developer still must make sure to manage the group of PropertyManagers and use the correct one when a name or property name is needed but that feels a little less error-prone than managing a group of Matchers.

Or, I might have entirely missed the point of these methods, in which case please teach me!

@ljnelson
Copy link
Member Author

I understand the words but maybe not the intent describing the contract among getPropertyPatternMatcher and the getName and getPropertyName methods.

(Disclaimer and fair warning: I hoisted the extant AbstractDataSourceExtension class "up" and removed all the DataSource-y specific bits without further designing anything. But you have valid points all around here so yeah let's talk about them!)

The JavaDoc states that the latter two methods expect the caller to pass a Matcher that was previously returned from getPropertyPatternMatcher and which therefore was created specifically for the name provided to getPropertyPatternMatcher.

The original thought lo these many years ago was:

  • Keep things stateless as much as possible
  • Stop extensions from doing inefficient string matching/surgery as much as possible; standardize on precompiled regexes
  • Let the extension figure out which capturing group or other shenanigans it wants to use to figure out information from the scalar MicroProfile Config property as matched

But if there are multiple properties for a given extension (IIUC there could be quite a few), then the developer's code will have multiple Matcher instances to keep track of and will need to make sure to pass the correct Matcher instance to getName or getPropertyName at any point in the code.

Sorta kinda? You, the extension author, generate a Matcher however you want, when asked, given a scalar MicroProfile Config configuration property name (or you return null, if I remember right, if the property name in question is not appropriate for you to handle). You don't save it. Then that Matcher is handed back to you later when the orchestration logic decides that it is time for you to harvest a particular kind of "sub name" from the match.

So the orchestration hands you, let's say, "com.foo.Blatz.test.frobnicationInterval", in (as currently written) getPropertyPatternMatcher (a gross name; I'll probably change it, so thanks). You say, hmm, nope, I'm not an extension that supports injection of com.foo.Blatz types or subtypes, so you return null. Now the orchestration hands you "com.foo.Frob.prod.color", and you say, yep, I am responsible for com.foo.Frob instances, so I will create a Matcher that can "find" both prod and color if ever it is asked. Then the orchestration, receiving this non-null Matcher you just generated, hands it back to you in getName(Matcher), so you can (most likely) call, say, m.group(1) or something on it to get the name (prod) out of it. Then the orchestration hands it back to you again in getPropertyName(Matcher), so you can get the property name (color) out of it (via, say, m.group(2) or similar). Then the Matcher is discarded. The orchestration ends up with a Properties object built out of all this (containing color set to some value) indexed under the name (prod), and it is that Properties object that is handed to the addBean method where the real work happens.

This approach lets subclasses decide, for example, where they want to "find" the name and the property name (and any other supporting information), but forces (sort of) the extension to not go off into the weeds and do something inefficient and/or stupid. (Since the subclass author is normally me, I know my tendencies!)

I realize these are protected methods, and so are likely to be accessible only to someone implementing a configured extension. We can probably expect a good degree of care from such a developer.

Maybe!

Even so, with the existing approach the developer needs to get two things right: create and save the Matcher

No, there shouldn't be any saving going on. See initializeMasterProperties(), which is the orchestrator (and which is called from the afterBeanDiscovery container lifecycle event observer. Note that the Matcher in question is not retained anywhere, on purpose, so that when the extension lives forever (as extensions do) there's nothing occupying memory after it's done.

Finally, hunks of this were designed when we were also trying to auto-connect to ACCS services as defined in the environment in some cases. That requirement has left us, but some of the property initialization dance remains, and it might be useful down the road in another (modern) cloud services world.

@tjquinno
Copy link
Member

Looks as if I did miss the point. OK, that helps (I think!).

tjquinno
tjquinno previously approved these changes Feb 14, 2024
arjav-desai
arjav-desai previously approved these changes Feb 15, 2024
Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
…rts them to four leading spaces

Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
@ljnelson ljnelson merged commit c114db2 into helidon-io:main Feb 15, 2024
12 checks passed
hrstoyanov pushed a commit to hrstoyanov/helidon that referenced this pull request Feb 23, 2024
…-io#8378)

Adds support for injecting UniversalConnectionPool instances

Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x Version 4.x cdi CDI enhancement New feature or request integration java Pull requests that update Java code jpa/jta MP OCA Verified All contributors have signed the Oracle Contributor Agreement. P4
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support injecting UniversalConnectionPool and UniversalConnectionPoolManager instances directly in CDI
3 participants