Skip to content

Conversation

@eric-maynard
Copy link
Contributor

When debugging with Quarkus, @RichardLiu2001 and I noticed that DefaultActiveRolesProvider doesn't have an Identifier currently. This means that if we add a new implementation of ActiveRolesProvider, the runtime can fail with:

jakarta.enterprise.inject.AmbiguousResolutionException: Ambiguous dependencies for type org.apache.polaris.service.auth.ActiveRolesProvider and qualifiers [@Default]

Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

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

This change is innocuous in the current codebase. The added identifier could be used to look up an ActiveRolesProvider by custom producers in environments where multiple implementations are available.

@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Mar 5, 2025
@dimas-b
Copy link
Contributor

dimas-b commented Mar 5, 2025

My mistake, as CI shows the change is not innoccuous 😅

UnsatisfiedResolutionException: Unsatisfied dependency for type org.apache.polaris.service.auth.ActiveRolesProvider and qualifiers [@Default]

This can be resolved by adding a custom producer method similar to this:

return metaStoreManagerFactories.select(Identifier.Literal.of(persistenceType)).get();


@Produces
public ActiveRolesProvider activeRolesProvider(
@ConfigProperty(name = "quarkus.active-roles-provider.type") String persistenceType,
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably need to set this config in application.properties to default

@eric-maynard
Copy link
Contributor Author

eric-maynard commented Mar 5, 2025

Good catch @dimas-b, did you mean something like this?

What I see now when running QuarkusProducers is:

System property not set correctly: quarkus.http.test-port
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)

edit: solved by adding the config to application.properties

@eric-maynard eric-maynard requested a review from dimas-b March 5, 2025 18:29
@eric-maynard eric-maynard enabled auto-merge (squash) March 5, 2025 18:29
@dimas-b
Copy link
Contributor

dimas-b commented Mar 5, 2025

System property not set correctly: quarkus.http.test-port

This is usually an indicator that the server did not start for some reason... The root cause may be hidden in logs (especially if they are redirected).

Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@eric-maynard eric-maynard merged commit 1017561 into apache:main Mar 5, 2025
5 checks passed
@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Mar 5, 2025
eric-maynard added a commit to eric-maynard/polaris that referenced this pull request Mar 13, 2025
…apache#24)

* add identifier

* add producer

* add to config

Co-authored-by: Eric Maynard <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants