Re: test classes - I'd propose to make a new module, e.g. `polaris-test` to contain abstract (actually or in spirit) test code, which would then be picked up by DW and Quarkus-specific actual test classes.
I'm sure we can isolate env. details in JUnit5 extensions and inject relevant access objects (e.g. REST clients / URLs / etc) in runtime. Cheers, Dmitri. On Wed, Nov 13, 2024 at 2: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 > > > >> > > > > > > >
