Working impl of HK2 dependency injection #493

collado-mike · 2024-11-27T21:17:41Z

Description

As discussed in the mailing list, this change proposes using the HK2 framework to support CDI-compliant dependency injection using the existing Dropwizard application structure and yaml configuration. This change does not make a logic changes or signature changes except to remove some of the injection-oriented interfaces (e.g., ConfigurationStoreAware) and updates constructors to accept dependencies via @Inject annotations.

The PolarisApplicationConfig is configured to be managed by HK2, but populated by the Jackson during the typical Dropwizard configuration parsing. Because the PolarisApplicationConfig is managed by HK2, it can invoke a @PostConstruct method that registers configured beans with the serviceLocator using the config class's own getter methods as Factory implementations. Because the other beans are not managed by HK2 (they can't be, since they declare dependencies on other beans that may not yet be instantiated by Jackson), the factory manually calls inject on each bean before returning.

Jersey has its own serviceLocator instance under the hood, so all of the beans bound to the PolarisApplicationConfig instance are registered again with the Jersey serviceLocator so that they can be found by the jersey components at runtime (I tried using the HK2 bridge that supposedly allows connecting two serviceLocators, but it didn't seem to work and also the licensing on that package suggests possible GPL violation).

This also introduces a @RealmScope that declares certain dependencies to be created and managed per-realm, rather than per-application or per-request. Some of the instances, such as the PolarisMetaStoreManager and the EntityCache are intended to be reused across requests, but of course, cannot be reused across realms, as some entities (such as root principal and service_admin principal role) have the same name across realms.

One additional test class is added to ensure that the @RealmScoped EntityCache is reused across requests, but is not reused across different realms.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
Documentation update
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

PolarisRealmEntityCacheTest

Test Configuration:

Hardware:
Toolchain:
SDK:

Checklist:

Please delete options that are not relevant.

I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
If adding new functionality, I have discussed my implementation with the community using the linked GitHub issue

dimas-b

Just a couple of quick comments... I'll do a full review a bit later :)

...s-service/src/main/java/org/apache/polaris/service/persistence/cache/EntityCacheFactory.java

extension/persistence/eclipselink/build.gradle.kts

snazy

Thanks for the nice improvement towards CDI!

hk2 adds a DI implementation, but in a couple of places this conflicts with the way Jackson is used as a DI-ish mechanism to instantiate things, leading to multiple DI-(ish) implementations.

I'm not sure how an approach to add "generic" extensions/plugins would look with hk2, as we cannot expect to have initialization code for all extensions/plugins in the "main" code base.

polaris-core/build.gradle.kts

polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java

...is-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegrationProvider.java

snazy · 2024-12-02T12:18:16Z

polaris-service/src/main/java/org/apache/polaris/service/PolarisApplication.java

+            new AbstractBinder() {
+              @Override
+              protected void configure() {
+                bind(RealmScopeContext.class)


Looks like there's a lot of manual coding needed for hk2, which feels like it makes things complicated, when we eventually have a plugin/extension mechanism.

I think this should be considered the smallest change necessary to get CDI working. There are improvements that can be made, e.g., classpath scanning for detecting Resource and Service impls, but the primary goal right now is to have runtime framework dependent on @Inject for wiring. Once that's done, lots of other changes can happen in parallel - e.g., improvements to the authorizer, authentication changes, as well as improvements to the CDI framework. I think this accomplishes the smallest change necessary to allow those other changes to progress.

polaris-service/src/main/java/org/apache/polaris/service/PolarisApplication.java

polaris-service/src/main/java/org/apache/polaris/service/config/PolarisApplicationConfig.java

...main/java/org/apache/polaris/service/persistence/InMemoryPolarisMetaStoreManagerFactory.java

polaris-service/src/main/java/org/apache/polaris/service/tracing/TracingFilter.java

polaris-service/src/main/resources/META-INF/hk2-locator/default

snazy · 2024-12-02T12:47:10Z

regtests/Dockerfile

@@ -54,4 +54,4 @@ USER root
 RUN chmod -R go+rwx /home/spark/regtests
 USER spark

-CMD ["./run.sh"]
+ENTRYPOINT ["./run.sh"]


Change to this PR?

added when I was debugging this test failure- e5367d9 . TBH, I'm not sure how tests in main were passing

dimas-b · 2024-12-03T05:30:01Z

polaris-service/src/main/java/org/apache/polaris/service/config/PolarisApplicationConfig.java

+    public void dispose(T instance) {}
+  }
+
+  public <T> T findService(Class<T> serviceClass) {


This seems to be a CDI concern rather than a configuration concern. I mean that it is perfectly fine to drive CDI with config settings, but findService does not appear to fit the DI paradigm 🤔

It doesn't, but the servlet filters don't use DI, so the PolarisApplication has to find them somehow

dimas-b · 2024-12-03T05:39:30Z

polaris-service/src/main/resources/META-INF/hk2-locator/default

+[org.apache.polaris.service.auth.TestInlineBearerTokenPolarisAuthenticator]S
+contract={io.dropwizard.auth.Authenticator}
+name=test
+qualifier={jakarta.inject.Named}


I'm not sure I understand the purpose of this declaration. It seems rather redundant to me. The class of TestInlineBearerTokenPolarisAuthenticator already has the @Named("test") annotation and implements Authenticator. This file does not appear to add any new information to what is already known from class files 🤔

This is something that would normally be generated at build time by scanning the build artifacts. But that requires using the HK2-specific annotations (@Contract and @Service), which I think we agreed to avoid

snazy · 2024-12-03T17:14:32Z

Hm, looking into the hk2 github repo it looks like its rather unmaintained (minus version bumps and rather tiny changes).

dimas-b

Overall this PR looks good to me as it enables CDI and paves the way to further improvements / simplifications 👍

dimas-b · 2024-12-03T19:08:15Z

I'd also be willing to work on extracting services (generated API code + impl. classes) into a dedicated gradle module (to be pulled into DW runtime), unless you want to work on this from your side @collado-mike :)

dimas-b · 2024-12-03T19:18:04Z

polaris-service/src/main/java/org/apache/polaris/service/context/RealmScopeContext.java

+
+  @SuppressWarnings("unchecked")
+  @Override
+  public <U> U findOrCreate(ActiveDescriptor<U> activeDescriptor, ServiceHandle<?> root) {


Is it possible to implement @RealmScope as a @RequestScoped with custom bean producers that handle the realm lookup and caching? I think that might be more intuitive from the CDI perspective... and probably more portable. WDYT?

I'd worry that the intention was not clear - readers would just have to know that an item annotated with @RequestScoped could actually live beyond the scope of a single request. In fact, they could be used by multiple concurrent requests, while the @RequestScoped annotation would convey that it's safe for an object to mutate state possibly leading to thread safety errors. I think all of the CDI providers offer support for custom scopes and it doesn't seem like a lot of work for what amounts to (IMO) clear intentions.

good point about concurrent access

I think we are mixing concepts here, a request scoped bean should not outlive the request. In the Quarkus PR the RealmContext is a request-scoped bean, and one instance of it is created for each request. It two requests target the same realm, they will get two distinct instances of RealmContext, but each would have the same realm ID.

We're specifically talking about beans that should be reused across requests

Why not have a global store for realm-specific stuff and expose it to callers via thin request-scoped wrappers? Disposing of the wrapper does not have to dispose of the long-lived data.

... besides, we never destroy realm-scoped beans anyway.

It seems to me that's exactly what the @RealmScoped annotation is doing. The RealmScopeContext maintains the cache per-realm and manages the instantiated values for each ActiveDescriptor for the currently active realm. This is a centralized place where we can do a lot of cleanup of stale realms (e.g., somebody made a call two days ago, but hasn't invoked an API since), rather than having to sprinkle this cache eviction in a bunch of different bean-specific factories.

I think the contention is whether we should be relying on a ThreadLocal to access the current RealmContext. I can see where there are flaws with this approach (e.g., we already ran into one during the asynchronous cleanup of metadata files after table drop), but at the same time, I think most (every?) implementation of @RequestScoped relies on ThreadLocal variables (e.g., AFAICT, ArC relies on ThreadLocalCurrentContext in its RequestContext ). Injecting an accessor to retrieve the current realm makes a lot of sense to me, but at the end of the day, I think we're still going to be relying on ThreadLocals.

Request scopes are probably implemented on top of some thread locals, but the important point here is that whatever mechanism is used to store and retrieve contextual data, it has to be managed by the runtime.

…-scoped services

…iceLocator and handle DI for configured instances

…tion header

collado-mike · 2024-12-03T23:05:50Z

I'd also be willing to work on extracting services (generated API code + impl. classes) into a dedicated gradle module (to be pulled into DW runtime), unless you want to work on this from your side @collado-mike :)

That sounds good to me! I have other things to work on :)

polaris-core/src/main/java/org/apache/polaris/core/context/RealmScope.java

adutra · 2024-12-04T10:50:30Z

polaris-service/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java

@@ -39,8 +40,8 @@
 * Default implementation of the {@link OAuth2ApiService} that generates a JWT token for the client
 * if the client secret matches.
 */
-@JsonTypeName("default")
-public class DefaultOAuth2ApiService implements OAuth2ApiService, HasMetaStoreManagerFactory {
+@Named("default")


I agree that @Named has some shortcomings. From CDI spec 3.0:

The use of @Named as an injection point qualifier is not recommended, except in the case of integration with legacy code that uses string-based names to identify beans.

One of them is that the name is application-wide, so you cannot have two beans named @Named("default").

Instead, we could use some other qualifier. People generally use @Identifier from Smallrye-Common library. The Quarkus PR is using that for now.

This article explains the pros and cons of each annotation:

https://dev.to/yanev/unveiling-challenges-with-named-67p

polaris-service/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java

polaris-service/src/main/java/org/apache/polaris/service/auth/DefaultPolarisAuthenticator.java

polaris-service/src/main/java/org/apache/polaris/service/auth/JWTRSAKeyPairFactory.java

polaris-service/src/main/java/org/apache/polaris/service/auth/JWTSymmetricKeyFactory.java

polaris-service/src/main/java/org/apache/polaris/service/context/DefaultContextResolver.java

adutra · 2024-12-04T11:27:42Z

...s-service/src/main/java/org/apache/polaris/service/persistence/cache/EntityCacheFactory.java

+  private static Logger LOGGER = LoggerFactory.getLogger(EntityCacheFactory.class);
+  @Inject PolarisMetaStoreManager metaStoreManager;
+
+  @RealmScope


Again I think it's much easier to turn this bean into application-scoped and have it hold a map of entity caches keyed by realm id, like RealmEntityManagerFactory.

...ore/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegrationProviderImpl.java

adutra · 2024-12-04T11:30:26Z

polaris-service/src/main/java/org/apache/polaris/service/BootstrapRealmsCommand.java

@@ -48,17 +46,11 @@ protected void run(
      Bootstrap<PolarisApplicationConfig> bootstrap,
      Namespace namespace,
      PolarisApplicationConfig configuration) {
-    MetaStoreManagerFactory metaStoreManagerFactory = configuration.getMetaStoreManagerFactory();
+    MetaStoreManagerFactory metaStoreManagerFactory =


Commands (boostrap/purge) are going to be fun to adapt to CDI. I suggest we look into them later but my feeling is that we would need to move them to separate modules.

server-templates/api.mustache

collado-mike · 2024-12-04T19:14:27Z

I agree that @Named has some shortcomings.
One of them is that the name is application-wide, so you cannot have two beans named @Named("default").

@adutra I haven't seen this behavior during testing. E.g., the DefaultOAuth2ApiService and the DefaultContextResolver are both named "default", but the application doesn't seem to have any problem starting up both instances. Could it be that we never use @Named as a qualifier at the injection point? From the article you shared, it seems like the issue is one of ambiguity at injection time if the qualifiers aren't specific. But the goal of the @Named annotation in this PR is to allow named instances to be specified in the configuration. We never use the @Named qualifier at the injection points (and I don't think we should, as it defeats the purpose of the user configuration).

adutra · 2024-12-05T11:22:43Z

@adutra I haven't seen this behavior during testing. E.g., the DefaultOAuth2ApiService and the DefaultContextResolver are both named "default", but the application doesn't seem to have any problem starting up both instances.

I suspect HK2 might not be fully compliant with the spec. From CDI spec 4.0:

An ambiguous name exists when a name resolves to multiple beans. When an ambiguous name exists, the container attempts to resolve the ambiguity.

So in our case, if both DefaultOAuth2ApiService and DefaultContextResolver are named default, and are both eligible for injection, then that should result in an unresolvable ambiguous bean situation.

For example with Quarkus, if I declare the following beans:

@Singleton @Named("foo")
public class Foo1 {}

@Singleton @Named("foo")
public class Foo2 {}

I get a deployment error when the app starts (even if the beans aren't injected anywhere):

java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.arc.deployment.ArcProcessor#generateResources threw an exception: jakarta.enterprise.inject.spi.DeploymentException: Unresolvable ambiguous bean name detected: foo
Beans:
CLASS bean [types=[org.apache.polaris.service.catalog.Foo1, java.lang.Object], qualifiers=[@Default, @Any, @Named("foo")], target=org.apache.polaris.service.catalog.Foo1]
CLASS bean [types=[org.apache.polaris.service.catalog.Foo2, java.lang.Object], qualifiers=[@Default, @Any, @Named("foo")], target=org.apache.polaris.service.catalog.Foo2]

To get past this issue, I would suggest that we copy SmallRye's @Identifier into our code base – if it's possible for HK2 to process custom qualifiers.

collado-mike · 2024-12-06T17:13:20Z

To get past this issue, I would suggest that we copy SmallRye's @Identifier into our code base – if it's possible for HK2 to process custom qualifiers.

Yeah, it's possible to use the custom qualifiers. I think it's fine to switch the annotations - but copying the @Identifier code will make the classes that use it incompatible with the Quarkus work, right? I think it would be fine to just add a dependency on the SmallRye library for the annotation, as that moves the codebase into a more compatible state. WDYT?

adutra · 2024-12-06T17:38:08Z

Yeah, it's possible to use the custom qualifiers. I think it's fine to switch the annotations - but copying the @Identifier code will make the classes that use it incompatible with the Quarkus work, right? I think it would be fine to just add a dependency on the SmallRye library for the annotation, as that moves the codebase into a more compatible state. WDYT?

Copying wouldn't break Quarkus afaict, it's just a matter of telling it which identifier we intend to use. That said, if you are OK adding the dependency, that's even better 👍

… with field injection

collado-mike · 2024-12-06T19:14:40Z

@adutra , I think I've addressed all of your comments. The one outstanding concern is the support for the @RealmScoped annotation. I think we should discuss the feasibility of supporting that in Quarkus in the Quarkus PR. Its use and the context to support it are framework-dependent anyway, so I think we can evaluate the feasibility of it specifically in Quarkus separately from this PR.

In the meantime, however, I think the basic CDI support is blocking work on several other tasks (e.g., the authentication/authorization PRs Dmitri and I have open, fixing the bootstrapping, table metadata logging). With this PR, I think we can work on better/more complete CDI support (e.g., through Quarkus) without any significant changes to the work done here and it can also accommodate the other work that's ongoing.

adutra

I'm approving in the spirit of allowing us to move forward. There will be issues to solve for sure, but I think we can fix them along the way. 👍

collado-mike · 2024-12-09T19:34:34Z

@adutra , I'll need another approval because I had to merge from main to resolve conflicts 🙄

collado-mike requested review from jbonofre, ashvina, RussellSpitzer, snazy, vvcephei, takidau, jackye1995, flyrain, eric-maynard, ebyhr and adutra as code owners November 27, 2024 21:17

dimas-b reviewed Nov 27, 2024

View reviewed changes

...s-service/src/main/java/org/apache/polaris/service/persistence/cache/EntityCacheFactory.java Show resolved Hide resolved

extension/persistence/eclipselink/build.gradle.kts Outdated Show resolved Hide resolved

snazy reviewed Dec 2, 2024

View reviewed changes

collado-mike mentioned this pull request Dec 2, 2024

Rework PolarisAuthorizer to use self-contained manifest on input #499

Closed

dimas-b reviewed Dec 3, 2024

View reviewed changes

dimas-b approved these changes Dec 3, 2024

View reviewed changes

dimas-b reviewed Dec 3, 2024

View reviewed changes

collado-mike added 11 commits December 3, 2024 12:20

Working impl of HK2 dependency injection

795416d

Add RealmScope and make EntityCache and PolarisMetaStoreManager realm…

8361fd8

…-scoped services

Remove dependency on HK2 from core module

578ff76

Fix EntityCache to work with RealmScope

7df1038

fix TimedApplicationEventListener.class

ab1e19d

Add integration test to validate EntityCache in RealmScope

ccb710f

Added comments on PolarisApplication startup

e01a66a

Update HK2 setup to use PolarisApplicationConfig as interface to serv…

080113e

…iceLocator and handle DI for configured instances

Fixed RealmEntityManagerFactory to use EntityCache provider

c4950ae

Update Dockerfile to use entrypoint and fix vended-credentials delega…

9763f0b

…tion header

fix spotless

46abf02

collado-mike added 3 commits December 3, 2024 12:25

Update build versions to include jakarta.inject

88b960e

Removed unnecessary Discoverable service files

73ca1ed

Fixes for rebase on main

2f60a7b

collado-mike force-pushed the mcollado-hk2-di branch from 377cbde to 2f60a7b Compare December 3, 2024 21:31

dimas-b reviewed Dec 4, 2024

View reviewed changes

polaris-core/src/main/java/org/apache/polaris/core/context/RealmScope.java Outdated Show resolved Hide resolved

Update Jersey ServiceLocator to use bootstrapped locator as parent

43640c3

adutra reviewed Dec 4, 2024

View reviewed changes

Addressed more PR comments

a3d6141

Update RealmScopeContext to use request-scoped RealmContext bean

5b2182d

sfc-gh-mcollado added 4 commits December 6, 2024 10:44

Update CDI impls to use @Identifier and replaced all setter injection…

25e130c

… with field injection

Fix helm test to match expectation

e7928fc

Revert PolarisStorageIntegrationProviderImpl back to service module

492f3bf

Fix missing project mentions in LICENSE

705221c

sfc-gh-mcollado added 2 commits December 6, 2024 11:27

Fix lint issue in helm values

2587138

Add logic in build to merge multiple hks-locator files

2190330

adutra approved these changes Dec 9, 2024

View reviewed changes

Merge branch 'main' into mcollado-hk2-di

30a404a

collado-mike enabled auto-merge (squash) December 9, 2024 19:03

collado-mike disabled auto-merge December 9, 2024 19:04

collado-mike enabled auto-merge (squash) December 9, 2024 19:31

adutra approved these changes Dec 9, 2024

View reviewed changes

collado-mike merged commit 9809e0e into apache:main Dec 9, 2024
5 checks passed

collado-mike deleted the mcollado-hk2-di branch December 19, 2024 00:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Working impl of HK2 dependency injection #493

Working impl of HK2 dependency injection #493

collado-mike commented Nov 27, 2024

dimas-b left a comment

snazy left a comment

snazy Dec 2, 2024

collado-mike Dec 2, 2024

snazy Dec 2, 2024

collado-mike Dec 3, 2024

dimas-b Dec 3, 2024

collado-mike Dec 4, 2024

dimas-b Dec 3, 2024

collado-mike Dec 4, 2024

snazy commented Dec 3, 2024

dimas-b left a comment

dimas-b commented Dec 3, 2024

dimas-b Dec 3, 2024 •

edited

Loading

collado-mike Dec 3, 2024

dimas-b Dec 4, 2024

adutra Dec 4, 2024

collado-mike Dec 4, 2024

dimas-b Dec 4, 2024

collado-mike Dec 4, 2024

adutra Dec 5, 2024

collado-mike commented Dec 3, 2024

adutra Dec 4, 2024

adutra Dec 4, 2024

adutra Dec 4, 2024

collado-mike commented Dec 4, 2024 •

edited

Loading

adutra commented Dec 5, 2024 •

edited

Loading

collado-mike commented Dec 6, 2024

adutra commented Dec 6, 2024 •

edited

Loading

collado-mike commented Dec 6, 2024

adutra left a comment

collado-mike commented Dec 9, 2024

Working impl of HK2 dependency injection #493

Working impl of HK2 dependency injection #493

Conversation

collado-mike commented Nov 27, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

dimas-b left a comment

Choose a reason for hiding this comment

snazy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snazy commented Dec 3, 2024

dimas-b left a comment

Choose a reason for hiding this comment

dimas-b commented Dec 3, 2024

dimas-b Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

collado-mike commented Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

collado-mike commented Dec 4, 2024 • edited Loading

adutra commented Dec 5, 2024 • edited Loading

collado-mike commented Dec 6, 2024

adutra commented Dec 6, 2024 • edited Loading

collado-mike commented Dec 6, 2024

adutra left a comment

Choose a reason for hiding this comment

collado-mike commented Dec 9, 2024

dimas-b Dec 3, 2024 •

edited

Loading

collado-mike commented Dec 4, 2024 •

edited

Loading

adutra commented Dec 5, 2024 •

edited

Loading

adutra commented Dec 6, 2024 •

edited

Loading