+1 from mv POV to merge
On Tue, Aug 5, 2025 at 1:58 PM Alexandre Dutra <adu...@apache.org> wrote: > > Hello all, > > We would like to merge [2233] sooner than later. Is it OK to merge today? > > Thanks, > Alex > > [2233] https://github.com/apache/polaris/pull/2233 > > On Fri, Aug 1, 2025 at 2:18 PM Alexandre Dutra <adu...@apache.org> wrote: > > > > Hi all, > > > > Here is the PR: > > > > https://github.com/apache/polaris/pull/2233 > > > > Thanks, > > Alex > > > > On Fri, Aug 1, 2025 at 12:51 PM Robert Stupp <sn...@snazy.de> wrote: > > > > > > WFM > > > > > > On Fri, Aug 1, 2025 at 12:45 PM Alexandre Dutra <adu...@apache.org> wrote: > > > > > > > > Hi Robert, > > > > > > > > I would very much like to "split the monolith" as well :-) But: > > > > > > > > 1) I think it would be easier to do it when the 2 modules are merged > > > > together since they share a great deal of closely related classes. > > > > 2) I am not sure everybody is on board with the split. I think we will > > > > need a new DISCUSS thread for that and maybe even a proposal doc. I'd > > > > suggest tackling that after the merge. > > > > > > > > Does that make sense? > > > > > > > > Thanks, > > > > Alex > > > > > > > > On Fri, Aug 1, 2025 at 12:31 PM Robert Stupp <sn...@snazy.de> wrote: > > > > > > > > > > I think we should rather use the opportunity and split the multiple > > > > > monoliths into distinct modules, which is what I've been advocating > > > > > for a "long" time ;) > > > > > > > > > > Regarding naming/where, I think we should retain "runtime" as a > > > > > container for the server and admin-tool runnables. > > > > > > > > > > WDYT? > > > > > > > > > > On Fri, Aug 1, 2025 at 12:12 PM Alexandre Dutra <adu...@apache.org> > > > > > wrote: > > > > > > > > > > > > Hi again, > > > > > > > > > > > > I understand that we haven't reached a full consensus on this topic > > > > > > yet. But since some people showed interest in seeing it > > > > > > implemented, I > > > > > > will go ahead and prepare the PR so that we can see what it would > > > > > > look > > > > > > like. > > > > > > > > > > > > More specifically I will: > > > > > > > > > > > > - Merge the 2 modules > > > > > > - Rename all the packages in the resulting module having a > > > > > > ".quarkus." token > > > > > > - Rename all the classes in the resulting module having a "Quarkus" > > > > > > prefix > > > > > > > > > > > > Thanks, > > > > > > Alex > > > > > > > > > > > > On Thu, Jul 31, 2025 at 8:11 PM Alexandre Dutra <adu...@apache.org> > > > > > > wrote: > > > > > > > > > > > > > > Hi Dmitri, > > > > > > > > > > > > > > Thanks for your reply. My proposal doesn’t affect how > > > > > > > polaris-runtime-defaults is “pulled in” by downstream builds. > > > > > > > > > > > > > > We can, however, explore ways to make the downstream integration > > > > > > > experience better, but imo only *after* the merge. > > > > > > > > > > > > > > Thanks, > > > > > > > Alex > > > > > > > > > > > > > > Le jeu. 31 juil. 2025 à 19:56, Dmitri Bourlatchkov > > > > > > > <di...@apache.org> a écrit : > > > > > > >> > > > > > > >> Hi Alex, > > > > > > >> > > > > > > >> Unifying polaris-service-common and polaris-runtime-service > > > > > > >> sounds good to > > > > > > >> me. > > > > > > >> > > > > > > >> Re: config, I suppose it should not be an issue to have Quarkus > > > > > > >> (or > > > > > > >> Smallrye) dependencies in any "service" modules. > > > > > > >> > > > > > > >> Side note: I'd like to exclude polaris-runtime-defaults from the > > > > > > >> transitive > > > > > > >> dependency chain and only depend on it directly from leaf > > > > > > >> runtime artifacts > > > > > > >> (admin and server). The reason for this is to simplify > > > > > > >> downstream builds > > > > > > >> that reuse common services (having multiple > > > > > > >> application.properties in a > > > > > > >> Quarkus build env. is a nightmare :) ). I hope this will not > > > > > > >> interfere with > > > > > > >> your proposal. I'm mentioning it here only because it affects > > > > > > >> the polaris-runtime-service module (IIRC). > > > > > > >> > > > > > > >> Cheers, > > > > > > >> Dmitri. > > > > > > >> > > > > > > >> On Thu, Jul 31, 2025 at 1:36 PM Alexandre Dutra > > > > > > >> <adu...@apache.org> wrote: > > > > > > >> > > > > > > >> > > I mentioned smallrye-config, not Quarkus, so you _can_ > > > > > > >> > > annotate those. > > > > > > >> > > > > > > > >> > Fair point: you are absolutely right. We *could* do things the > > > > > > >> > other > > > > > > >> > way around, and move the configuration classes from the > > > > > > >> > polaris-runtime-service module to the polaris-service-common > > > > > > >> > module. > > > > > > >> > > > > > > > >> > BUT: my main point for proposing this change still holds: *the > > > > > > >> > two > > > > > > >> > modules do not have a clearly defined purpose*. Both have code > > > > > > >> > for > > > > > > >> > e.g. authentication, events, persistence, storage, task > > > > > > >> > handling, etc. > > > > > > >> > etc. > > > > > > >> > > > > > > > >> > This doesn't look to me as a desirable state. I think that if > > > > > > >> > classes > > > > > > >> > that perform the same actions could live next to each other, > > > > > > >> > that > > > > > > >> > would be beneficial to the developer experience. > > > > > > >> > > > > > > > >> > Thanks, > > > > > > >> > Alex > > > > > > >> > > > > > > > >> > On Thu, Jul 31, 2025 at 6:46 PM Robert Stupp <sn...@snazy.de> > > > > > > >> > wrote: > > > > > > >> > > > > > > > > >> > > > That's not possible because there are no Quarkus > > > > > > >> > > > dependencies in that > > > > > > >> > module, so you can't annotate with @WithDefault, for example. > > > > > > >> > > > > > > > > >> > > I mentioned smallrye-config, not Quarkus, so you _can_ > > > > > > >> > > annotate those. > > > > > > >> > > > > > > > > >> > > On Thu, Jul 31, 2025 at 6:34 PM Yufei Gu > > > > > > >> > > <flyrain...@gmail.com> wrote: > > > > > > >> > > > > > > > > > >> > > > +1. Thanks Alex for driving this. Other than the benefits > > > > > > >> > > > discussed > > > > > > >> > like > > > > > > >> > > > removing the duplicated config classes like > > > > > > >> > > > `QuarkusStorageCredentialCacheConfig`, or removing > > > > > > >> > > > `.quarkus.` in the > > > > > > >> > > > package name, we can finally put classes like > > > > > > >> > > > S3AccessConfig > > > > > > >> > > > and StsClientsPool into the right package. > > > > > > >> > > > > > > > > > >> > > > Yufei > > > > > > >> > > > > > > > > > >> > > > > > > > > > >> > > > On Thu, Jul 31, 2025 at 8:45 AM Eric Maynard > > > > > > >> > > > <eric.w.mayn...@gmail.com > > > > > > >> > > > > > > > > >> > > > wrote: > > > > > > >> > > > > > > > > > >> > > > > The project already *has* adopted Quarkus for good, so I > > > > > > >> > > > > think > > > > > > >> > making the > > > > > > >> > > > > module structure reflect that is fine. In addition to the > > > > > > >> > configuration > > > > > > >> > > > > issue, which is significant, I think cutting down on the > > > > > > >> > > > > number of > > > > > > >> > modules > > > > > > >> > > > > we publish is a noble goal. > > > > > > >> > > > > > > > > > > >> > > > > —EM > > > > > > >> > > > > > > > > > > >> > > > > On Fri, Aug 1, 2025 at 00:43 Alexandre Dutra > > > > > > >> > > > > <adu...@apache.org> > > > > > > >> > wrote: > > > > > > >> > > > > > > > > > > >> > > > > > Hi Robert, > > > > > > >> > > > > > > > > > > > >> > > > > > > I think we can get away by having the smallrye-config > > > > > > >> > annotations on > > > > > > >> > > > > the > > > > > > >> > > > > > "parent" interface. > > > > > > >> > > > > > > > > > > > >> > > > > > That's not possible because there are no Quarkus > > > > > > >> > > > > > dependencies in > > > > > > >> > that > > > > > > >> > > > > > module, so you can't annotate with @WithDefault, for > > > > > > >> > > > > > example. > > > > > > >> > > > > > > > > > > > >> > > > > > Indeed merging those two modules would mean that we're > > > > > > >> > > > > > adopting > > > > > > >> > > > > > Quarkus for good, but I think that at this point, > > > > > > >> > > > > > nobody would > > > > > > >> > object. > > > > > > >> > > > > > > > > > > > >> > > > > > In this merge, we could also remove the ".quarkus." > > > > > > >> > > > > > bits from > > > > > > >> > package > > > > > > >> > > > > > names and remove the "Quarkus" prefix of many classes. > > > > > > >> > > > > > I think this > > > > > > >> > > > > > would result in a much more readable code, but that's > > > > > > >> > > > > > just my > > > > > > >> > opinion > > > > > > >> > > > > > :-) > > > > > > >> > > > > > > > > > > > >> > > > > > My long-term vision is that, after the merge, we could > > > > > > >> > > > > > also > > > > > > >> > consider > > > > > > >> > > > > > splitting the resulting "uber-module" into smaller, > > > > > > >> > concern-specific > > > > > > >> > > > > > modules like "polaris-service-auth", > > > > > > >> > > > > > "polaris-service-telemetry", > > > > > > >> > > > > > "polaris-service-events", etc. This approach would > > > > > > >> > > > > > make it much > > > > > > >> > > > > > simpler for integrators to implement their own > > > > > > >> > > > > > customizations (and > > > > > > >> > > > > > brings no downsides for regular users). > > > > > > >> > > > > > > > > > > > >> > > > > > But this needs to be done in two steps: first, merge > > > > > > >> > > > > > the current > > > > > > >> > two > > > > > > >> > > > > > modules; then, split the result. > > > > > > >> > > > > > > > > > > > >> > > > > > Thanks, > > > > > > >> > > > > > Alex > > > > > > >> > > > > > > > > > > > >> > > > > > On Thu, Jul 31, 2025 at 5:22 PM Robert Stupp > > > > > > >> > > > > > <sn...@snazy.de> > > > > > > >> > wrote: > > > > > > >> > > > > > > > > > > > > >> > > > > > > Is the issue here just the configuration types or is > > > > > > >> > > > > > > there more? > > > > > > >> > > > > > > For the config types, I think we can get away by > > > > > > >> > > > > > > having the > > > > > > >> > > > > > > smallrye-config annotations on the "parent" > > > > > > >> > > > > > > interface. > > > > > > >> > > > > > > > > > > > > >> > > > > > > My concern is that polaris-runtime-service is rather > > > > > > >> > > > > > > the Quarkus > > > > > > >> > > > > > specifics. > > > > > > >> > > > > > > CDI is great, just Quarkus isn't the only > > > > > > >> > > > > > > enterprise-CDI runtime. > > > > > > >> > > > > > > Spoiler: I do *not* think that Polaris should move > > > > > > >> > > > > > > away from > > > > > > >> > Quarkus. > > > > > > >> > > > > > > > > > > > > >> > > > > > > But for sole testing purposes Quarkus is quite > > > > > > >> > > > > > > expensive, too > > > > > > >> > > > > > > expensive IMO to make it a necessity for all tests. > > > > > > >> > > > > > > Sure, not all tests have to be `@QuarkusTest`s, but > > > > > > >> > > > > > > I could > > > > > > >> > imagine > > > > > > >> > > > > > > that there will be tests that do not need Quarkus > > > > > > >> > > > > > > are annotated > > > > > > >> > as > > > > > > >> > > > > > > such. > > > > > > >> > > > > > > > > > > > > >> > > > > > > On Thu, Jul 31, 2025 at 12:19 PM Alexandre Dutra < > > > > > > >> > adu...@apache.org> > > > > > > >> > > > > > wrote: > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > Hi all, > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > The polaris-service-common module is a > > > > > > >> > > > > > > > reminiscence of the > > > > > > >> > times > > > > > > >> > > > > where > > > > > > >> > > > > > > > we were still supporting two runtimes. > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > I think it has now become obsolete, and could be > > > > > > >> > > > > > > > merged into > > > > > > >> > > > > > > > polaris-runtime-service. > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > One annoyance that polaris-service-common brings > > > > > > >> > > > > > > > is with > > > > > > >> > > > > configuration > > > > > > >> > > > > > > > classes that have to exist in both modules (e.g. > > > > > > >> > > > > > > > AuthenticationConfiguration vs > > > > > > >> > QuarkusAuthenticationConfiguration). > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > Would we be OK with this merge? If so I'm happy to > > > > > > >> > > > > > > > provide the > > > > > > >> > PR. > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > Thanks, > > > > > > >> > > > > > > > Alex > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> >
