Hi,
> @RealmContext is to support beans that are scoped to the
> realm, not the request. E.g., the MetaStoreManagerFactory maintains a map
> of realm-specific PolarisMetaStoreManagers that are reused for every
> request for a realm. The scope is intended to be larger
> than @RequestScoped, but smaller than @ApplicationScoped.
As long as this solution works, I'm fine with it. I'd observe though that
beans like MetaStoreManagerFactory, which indeed are capable of managing
many realms, are natural good fits for the application scope. In the
Quarkus branch, this bean is annotated with @ApplicationScoped, while all
beans that use it and operate on a specific realm, are all annotated with
@RequestScoped.
I would _really_ prefer to avoid that if possible. The whole argument for
> using standardized annotations is that they can be used by multiple
> frameworks without being tied to any one.
To clarify my statement, I think that most consensual annotations would go
in the common module, and most notably, the @Inject annotation, as well as
@Context, @RequestScoped, @ApplicationScoped @Produces, @Vetoed, etc. When
it comes to selecting beans at runtime though, we either need to agree on a
common technique (knowing that Quarkus offers interesting possibilities,
but those are outside of CDI specs), or resort to specific code in runtime
modules – which I don't think would be that bad.
Using subclasses, it could look like the below example:
// common service module
@ApplicationScoped
public class DefaultFooFactory implements FooFactory { /* all business
logic here */ }
// dropwizard runtime module
@SomeBeanQualifier
public class DropwizardFooFactory extends DefaultFooFactory { /* empty
class */ }
// quarkus runtime module
@SomeOtherBeanQualifier
public class QuarkusFooFactory extends DefaultFooFactory { /* empty class
*/ }
Another option that avoids subclasses is to use a producer method instead,
e.g.:
// quarkus runtime module
@Produces @ApplicationScoped @SomeOtherBeanQualifier
public FooFactory fooFactory() {
return new DefaultFooFactory();
}
I definitely want
> to avoid duplicating test classes, as that's pretty much guaranteed to
> ensure that something gets broken and is uncaught because we lacked test
> coverage.
Agreed, it's certainly possible to commonize the tests with a bit of
effort. Andrew's PR is merged already; I think it's rather orthogonal to
the issue around duplicating tests. The new TestEnvironmentExtension is not
currently used in the Quarkus port, but it certainly could.
Thanks,
Alex
On Thu, Nov 14, 2024 at 6:53 PM Michael Collado <collado.m...@gmail.com>
wrote:
> > Although, I wonder if this custom
> context isn't identical to the standard request context? IOW, is it
> possible for the Polaris context to change during an HTTP call to Polaris
> API? I don't think so, which leads me to think that, at least in Quarkus,
> we'd use the standard @RequestScoped annotation for passing things like
> this.
>
> TBH, I'm a bit fuzzy on the details of the custom scopes in HK2, but the
> intention of the @RealmContext is to support beans that are scoped to the
> realm, not the request. E.g., the MetaStoreManagerFactory maintains a map
> of realm-specific PolarisMetaStoreManagers that are reused for every
> request for a realm. The scope is intended to be larger
> than @RequestScoped, but smaller than @ApplicationScoped. This is
> especially important for the EntityCache, which maintains a cache of the
> versioned entities and allows reuse of common entities (e.g., Catalogs and
> PrincipalRoles) without having to fetch them for every request. I _think_
> what I did with @RealmContext is to maintain a map of reusable beans that
> are not recreated for each request (the context itself is a singleton).
>
> > And to circle back to DI, whether this common module would contain
> classes
> annotated with Jakarta CDI annotations is imho up for debate. This would
> simplify things for sure, but on the flip side, would require both runtimes
> to use the exact same CDI mechanisms (e.g. both runtimes would use @Named
> beans). I think that this is going to be too restrictive. An alternative
> would be to keep the common classes clean of CDI annotations and let the
> runtime modules create subclasses annotated with their own annotations.
>
> I would _really_ prefer to avoid that if possible. The whole argument for
> using standardized annotations is that they can be used by multiple
> frameworks without being tied to any one. Personally, I'm not married to
> the @Named annotation or any of the specifics I used to get my prototype to
> work. If there is a more generalized annotation or alternative to
> specifying named instances of contracts, I'm all for it.
>
> Andrew Guterman <https://github.com/andrew4699> has been working on making
> the target test environment more pluggable (see
> https://github.com/apache/polaris/pull/391 ). Maybe we can get his input
> on
> how to structure the tests in a way that they can hit the target runtime
> without making them framework specific. This is super useful for, e.g.,
> testing in a CD pipeline where you might hit a docker image or k8s
> deployment to validate behavior before deploying to prod. I definitely want
> to avoid duplicating test classes, as that's pretty much guaranteed to
> ensure that something gets broken and is uncaught because we lacked test
> coverage.
>
> Mike
>
>
> On Thu, Nov 14, 2024 at 7:21 AM Jean-Baptiste Onofré <j...@nanthrax.net>
> wrote:
>
> > Hi
> >
> > Using @Named is fine with Quarkus. We can also create custom qualifier
> > (with Literal): named is actually a qualifier :)
> >
> > I think we discussed about the plan, and to my understanding:
> > 1. I open the Quarkus PR (as draft) to keep what we did
> > 2. We improve the current structure:
> > 2.1. polaris-core should not depend to dropwizard (remove discoverable
> > here)
> > 2.2. we create polaris-service-common containing all the beans,
> > without dependency to Dropwizard (or Quarkus). Related to that, itests
> > should go into polaris-service-itest.
> > 2.2. we create polaris-service-dropwizard where we take
> > polaris-service-common and do the beans injection with Dropwizard.
> > (2.3). we can create polaris-service-quarkus where we take
> > polaris-service-common and do the beans injection with Quarkus
> >
> > My standpoint is still the same: if the "clean refactoring" is a good
> > thing, I think it's not possible to have both
> > polaris-service-dropwizard and polaris-service-quarkus, for three
> > reasons:
> > - it doesn't bring any benefits for the users, only confusion (which
> > service should I use)
> > - it will be almost impossible to maintain (in sync)
> > - it will prevent to leverage extensions provided (especially by Quarkus)
> >
> > So, my proposal is to move forward on the refactoring (we have a
> > consensus here).
> > I will work with Michael to identify exactly the gaps between
> > polaris-service-dropwizard and polaris-service-quarkus (configuration,
> > extensions, ...).
> >
> > Thoughts ?
> >
> > Regards
> > JB
> >
> > On Wed, Nov 13, 2024 at 8:13 PM Alex Dutra
> > <alex.du...@dremio.com.invalid> wrote:
> > >
> > > Hi Michael, hi all,
> > >
> > > I had a look at your branch and am very pleased to see that it was
> > > relatively easy to get DI working with DW + HK2. Good news!
> > >
> > > Digging a bit into the details, I see that most beans are annotated
> with
> > > @Named now. This is probably OK for Quarkus too, although there are
> other
> > > techniques that allow to select a specific implementation at runtime.
> > >
> > > I also think that the @RealmContext annotation is very smart, and
> > resonates
> > > with how I handled that in Quarkus. Although, I wonder if this custom
> > > context isn't identical to the standard request context? IOW, is it
> > > possible for the Polaris context to change during an HTTP call to
> Polaris
> > > API? I don't think so, which leads me to think that, at least in
> Quarkus,
> > > we'd use the standard @RequestScoped annotation for passing things like
> > > this.
> > >
> > > But now taking a step back and looking at the big picture, I think that
> > > what we need now is a plan forward. Given that many of us voiced
> concerns
> > > around an all-or-nothing switch from DW to Quarkus (quite
> understandably
> > > so), and assuming that there is agreement to maintain both runtimes (DW
> > and
> > > Quarkus) for some time, I'd like to suggest the phased approach below:
> > >
> > > 1. Remove the dependency to DW from polaris-core – there is a PR for
> > > that already: https://github.com/apache/polaris/pull/435
> > > 2. Refactor polaris-service into 2 modules: e.g.
> > "polaris-service-common"
> > > and "polaris-service-dropwizard" – I believe Dmitri is already
> working
> > > on this
> > > 3. Introduce DI + HK2 in polaris-service-dropwizard – that would
> come
> > > from your branch basically
> > > 4. Introduce polaris-service-quarkus – that would come mostly from
> > JB's
> > > branch
> > >
> > > Regarding step #2, it seems we can easily put 80 to 90% of the actual
> > > polaris-service production classes into the common module. The
> dropwizard
> > > runtime module would contain just a handful of DW-specific classes.
> > >
> > > The biggest issue to solve with step #2 is imho test classes. Quarkus
> and
> > > DW have diverse requirements for tests, but we may still be able to
> > > refactor things using abstract classes or extensions, and reuse most of
> > > them. In any case, I think that if, at least at the beginning, test
> > classes
> > > are duplicated in each runtime module, that's acceptable until we
> figure
> > > out a better way.
> > >
> > > And to circle back to DI, whether this common module would contain
> > classes
> > > annotated with Jakarta CDI annotations is imho up for debate. This
> would
> > > simplify things for sure, but on the flip side, would require both
> > runtimes
> > > to use the exact same CDI mechanisms (e.g. both runtimes would use
> @Named
> > > beans). I think that this is going to be too restrictive. An
> alternative
> > > would be to keep the common classes clean of CDI annotations and let
> the
> > > runtime modules create subclasses annotated with their own annotations.
> > >
> > > And back to the @RealmContext topic, that's the other big aspect that
> we
> > > will need to solve (maybe step #5?): CallContext is heavily reliant on
> > > ThreadLocal, and that is not very CDI-friendly, as you've seen. For
> now,
> > I
> > > did a patchy workaround for Quarkus. But we'd need to refactor that
> code
> > > cleanly, either by creating a custom context as you did, or by making
> > > CallContext a request-scoped bean that is evaluated for each HTTP
> > request.
> > > Either way, it then needs to be propagated to other threads (e.g. in
> > > TaskExecutorImpl); for Quarkus, this will require using proper context
> > > propagation <https://quarkus.io/guides/context-propagation> (for DW,
> > maybe
> > > ThreadLocal is fine here). And since CallContext is in the core module,
> > we
> > > will likely need to remove the static methods getCurrentContext() and
> > > setCurrentContext() and retrieve the context only by scope resolution
> > > (which, in turn, may use ThreadLocal under the hood if that's
> appropriate
> > > for DW).
> > >
> > > If everybody is on board with the plan above, how about we create GH
> > issues
> > > for each step, and maybe even a GH Project to track progress on this
> > front
> > > and split the effort?
> > >
> > > Thanks,
> > >
> > > Alex
> > >
> > >
> > > On Sat, Nov 9, 2024 at 10:15 AM Jean-Baptiste Onofré <j...@nanthrax.net>
> > > wrote:
> > >
> > > > Hi Michael
> > > >
> > > > Thanks for the update !
> > > >
> > > > As discussed during the last community meeting:
> > > > 1. I will open the draft PR with Quarkus in the meantime
> > > > 2. I will take a look on your branch powered by Glassfish HK2 CDI
> > > > (give me a few days as I was travelling this week :))
> > > >
> > > > Thanks !
> > > > Regards
> > > >
> > > > On Fri, Nov 8, 2024 at 2:36 AM Michael Collado <
> collado.m...@gmail.com
> > >
> > > > wrote:
> > > > >
> > > > > FYI, I took a stab at seeing how Polaris would work with HK2 as a
> CDI
> > > > impl.
> > > > > I only spent yesterday and today on this, so it's not complete, but
> > it is
> > > > > functional and the tests pass :)
> > > > >
> > > > > I took a lot of the common ideas from the Quarkus branch (e.g.,
> > deleted
> > > > all
> > > > > the HasXXX and XXXAware interfaces), but kept the JSON/Yaml
> config. I
> > > > > figured out how to use the Dropwizard Yaml to specify the
> > > > > implementation of, e.g. the Authenticator and the
> > > > MetaStoreManagerFactory,
> > > > > but have the instances managed and injectable by HK2. The goal
> there
> > was
> > > > to
> > > > > just keep the existing configuration format, but change the impl
> > under
> > > > the
> > > > > hood. I'm not married to the idea and I'm interested to see if the
> > > > > jakarta.enterprise.inject.* annotations/interfaces that are used in
> > the
> > > > > Quarkus branch can make this simpler. However, I do think it would
> > ideal
> > > > if
> > > > > we can get it working with the existing Yaml configuration, at
> least
> > in
> > > > the
> > > > > short term.
> > > > >
> > > > > I did add support for a @RealmScope annotation to support
> restricting
> > > > items
> > > > > to a given realm, such as the EntityCache and the
> > PolarisGrantManager.
> > > > This
> > > > > allowed me to do things like hide the grant lookups from the
> > EntityCache
> > > > so
> > > > > that the Resolver doesn't have to pass around the
> > ResolvedPolarisEntity,
> > > > > but instead the grants are found from the cache without making it
> > overt
> > > > in
> > > > > the PolarisAuthorizer API. This was one of my original goals with
> > > > breaking
> > > > > up the PolarisMetaStoreManager API into multiple interfaces. Right
> > now,
> > > > > everything still ties back to the configured
> PolarisMetaStoreManager
> > > > > implementation, but eventually we can get to where the
> GrantManager,
> > > > > CredentialVendor, etc. can all be swapped out for different
> > > > implementations.
> > > > >
> > > > > Please take a look at the changes at
> > > > >
> > > >
> >
> https://github.com/collado-mike/polaris/compare/c0e8dae5182d33e046216510e2b02b7cf923ffe8...collado-mike:polaris:mcollado-hk2-di
> > > > > and let me know what you think.
> > > > >
> > > > > Mike
> > > > >
> > > > > On Tue, Nov 5, 2024 at 9:42 AM Michael Collado <
> > collado.m...@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > I added a table to the README with the differences that were
> called
> > > > out. I
> > > > > > added some details that I think are worth understanding better.
> > E.g.,
> > > > the
> > > > > > Json layout we added has specific custom functionality we wanted
> > for
> > > > > > supporting key/value pairs and the micrometer annotation was
> added
> > for
> > > > some
> > > > > > custom support we wanted aside from what is supported with the
> > default
> > > > > > dropwizard metrics support.
> > > > > >
> > > > > >
> > > > > >
> > > >
> >
> https://github.com/collado-mike/polaris/blob/14865a97ad8f790a9992432d79975c05ff5c36fa/polaris-service-quarkus/README.md
> > > > > >
> > > > > > I think it would be great if others could add to the table with
> > > > features
> > > > > > that would be impacted by the migration and to call out the level
> > of
> > > > effort
> > > > > > in both dropwizard and quarkus.
> > > > > >
> > > > > > Another possible consideration would be upgrading our Dropwizard
> > > > > > dependency to the latest development version. It may be the case
> > that
> > > > doing
> > > > > > so would address some of the targeted features with less effort
> in
> > > > > > migrating.
> > > > > >
> > > > > > Mike
> > > > > >
> > > > > > On Tue, Oct 29, 2024 at 5:50 AM Jean-Baptiste Onofré <
> > j...@nanthrax.net>
> > > > > > wrote:
> > > > > >
> > > > > >> Hi folks,
> > > > > >>
> > > > > >> Following the last community sync meeting, we create a branch
> > > > > >> demonstrating use of Quarkus to powered Apache Polaris:
> > > > > >>
> > > > > >> https://github.com/jbonofre/polaris/tree/QUARKUS
> > > > > >>
> > > > > >> We still have work to do, but you can already take a look and
> > > > > >> experiment (in the polaris-service-quarkus module).
> > > > > >>
> > > > > >> We added a README.md file to:
> > > > > >> 1. Highlight the main differences (in terms of code) between
> > > > > >> Dropwizard and Quarkus
> > > > > >> 2. To build and run Polaris powered by Quarkus
> > > > > >> 3. The list of TODO items
> > > > > >>
> > > > > >>
> > > > > >>
> > > >
> >
> https://github.com/jbonofre/polaris/blob/QUARKUS/polaris-service-quarkus/README.md
> > > > > >>
> > > > > >> If anyone wants to contribute on the branch before creating the
> > PR,
> > > > > >> please let me know, I will add you as contributor on the branch.
> > > > > >>
> > > > > >> Any comments or questions are welcome !
> > > > > >>
> > > > > >> Thanks,
> > > > > >> Regards
> > > > > >> JB
> > > > > >>
> > > > > >
> > > >
> >
>
