To avoid confusion: I mean `polaris-test` is for the integration kind of tests that require some support from the environment, like starting a Polaris server.
On Wed, Nov 13, 2024 at 4:27 PM Dmitri Bourlatchkov < dmitri.bourlatch...@dremio.com> wrote: > 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 >> > > >> >> > > > >> > >> >
