Matt, maybe I am not getting it but... I understand your explanation about Configurable-vs-Plugin, yet my question still stands: Can something be `@Configurable` but not `@Plugin`? If I am not mistaken, every single `@Configurable` usage is followed by a `@Plugin`. Hence, I am inclined to make `Configurable` extend from `Plugin`. This will not only simplify the call-site, but will communicate a more clear message: this is a plugin that will be configured. I can also reverse my question: What would be the benefit of keeping `@Configurable` and `@Plugin` separate?
On Sun, May 22, 2022 at 11:49 PM Matt Sicker <[email protected]> wrote: > Those lazy wrappers are an insignificant detail. Feel free to simplify. > > Configurable is a namespace annotation, not a name annotation. Plugin is a > name annotation that gets indexed. > > If you’d like fancier conditional annotations, please describe what parts > you want. I don’t want to port over every feature of Spring’s DI system as > it’s fairly extensive and mostly over complex for our use case. > > — > Matt Sicker > > > On May 22, 2022, at 15:36, Volkan Yazıcı <[email protected]> wrote: > > > > Great work Matt! I have some questions/remarks: > > > > 1. Why did we introduce `LazyInt`, `LazyBoolean`, etc. rather than > > simply leveraging `Lazy<V>`? If the concern is nullability, we could > have > > checked the `supplier` response against `null` in `Lazy<V>` itself. > > 2. Why is `Configurable` not annotated with `Plugin`? Can something be > > `@Configurable` but not `@Plugin`? > > 3. `ConditionalOnProperty` is missing `havingValue` and > `matchIfMissing`. > > > > > >> On Fri, May 20, 2022 at 11:37 PM Matt Sicker <[email protected]> wrote: > >> > >> By the way, I hope my latest commit renaming categories to namespaces > >> and moving some annotation metadata around should help clarify the > >> scope of things. In particular, I made an alias annotation for Core > >> category/namespace plugins called @Configurable which makes their use > >> case more obvious. Similarly, I moved the Core-specific annotation > >> data from @Plugin to @Configurable since they only apply there. Now > >> that the category is in the @Namespace annotation, @Plugin only has a > >> single value() string to set which is the name of the plugin (which > >> will default to using the simple class name if the plugin name is an > >> empty string; I've removed explicit plugin names on classes whose > >> simple names already match their plugin names). Note that you can > >> still define namespace alias annotations for different namespaces > >> (I've only made them for Core, Lookup, and TypeConverter namespaces so > >> far). > >> > >> Some next steps to make the system more consistent: > >> * @ConditionalOnFoo annotations for injectable bundles (i.e., the > >> equivalent to a @Configuration class in Spring) > >> * Ability to inject PluginType<T> for plugin namespaces that shouldn't > >> eagerly load available plugins > >> * Further cleanup and potential supporting APIs to apply inversion of > >> control rather than calling Injector APIs > >> * Figuring out how Injector may interact with or otherwise set up > >> things like PropertySource services or other log4j-api services. This > >> might be doable via a sort of "DI promise" API where the DI system > >> will complete the promise or some variation of lazy async loading > >> > >> From there, hopefully any remaining limitations or code smells will be > >> obvious enough to fix up before 3.0.0. > >> > >>> On Thu, May 19, 2022 at 3:06 PM Matt Sicker <[email protected]> wrote: > >>> > >>> I’d like to add more ConditionalOn annotations for the @Factory > methods, > >> or at least something like “if no binding exists for this key already, > here > >> it is” which is analogous to @ConditionalOnMissingBean in Spring. That > >> would make the DefaultCallback code easier to write without using the > >> Injector API directly. That will also make a good example for users to > copy > >> from when making their own InjectorCallback customizations. > >>> > >>> I do need to look more closely at the API module to see how injection > >> can work without pulling DI APIs up. Might need to define some > >> ServiceLoader stuff there like an ordering annotation. > >>> > >>> — > >>> Matt Sicker > >>> > >>> On May 19, 2022, at 10:47, Ralph Goers <[email protected]> > >> wrote: > >>> > >>> > >>> > >>>> On May 19, 2022, at 12:15 AM, Volkan Yazıcı <[email protected]> wrote: > >>> > >>> > >>> In the last couple of weeks, I have been interrogating Matt on why and > >> how > >>> > >>> of the 3.x plugin infra. This inevitably led to some code archaeology. > I > >>> > >>> will try to share my take out of this exercise. > >>> > >>> > >>> 1.x required users to type the fully-qualified class names of > components > >>> > >>> (appenders, layouts, etc.) in either configuration files or properties: > >>> > >>> > >>> log4j.appender.A1=org.apache.log4j.ConsoleAppender > >>> > >>> log4j.appender.A1.layout=org.apache.log4j.PatternLayout > >>> > >>> > >>> Plugins were first introduced in 2.x to address this concern: > >>> > >>> > >>> It wasn’t a concern. It was an annoyance. > >>> > >>> providing a > >>> > >>> mechanism to alias components. Later on it also got used to glue the > rest > >>> > >>> of the configuration components together. Consequently maintainers > >> started > >>> > >>> exercising this throughout the entire code base, whenever something > >> needed > >>> > >>> to be indirectly injected somewhere. > >>> > >>> > >>> Only where we thought it made sense. Some things use system properties, > >>> some things use ServiceLoader. > >>> > >>> > >>> > >>> That was a great epiphany and I am in love with the plugins! It feels > >> like > >>> > >>> Log4j is composed of simple beans beautifully practicing > >>> > >>> separation-of-concerns while running on a marvellous Spring-like > >>> > >>> fully-fledged dependency injection (DI) framework... almost... sort > of... > >>> > >>> The reality is sadly a little bit different than that. In particular, I > >> see > >>> > >>> two major issues: > >>> > >>> > >>> 1. Missing `@ConditionalOn*` support > >>> > >>> > >>> I am certain I added the ability to do this in 3.0. I added constraint > >> checking > >>> at the class level where previously it was only available on > parameters. > >> That > >>> said, we currently only have RequiredClass and RequiredProperty > >> validators. > >>> > >>> 2. Static access to DI > >>> > >>> > >>> I guess Matt is already working on issue #1. He is trying to make sure > >>> > >>> `@Required` et al. annotations are executed the same way at every > >> injection > >>> > >>> site. > >>> > >>> > >>> What do I mean with the static access to DI? In a `@Bean`-annotated > >> method > >>> > >>> of a Spring application, do you create your own `ApplicationContext` > >>> > >>> instance and access other beans from that? Do you statically access > >>> > >>> `ApplicationContext.getBean("foo")`? Certainly not! Though these two > >>> > >>> examples are what we exactly do in Log4j. We create single-use > >>> > >>> `PluginManager` instances and use it to collect plugins. We call > >>> > >>> `DI.createInjector().getInstance("foo")`. What we should be rather > doing > >> is > >>> > >>> to inject the `ApplicationContext` and/or the beans we are interested > in, > >>> > >>> that is, in Log4j terms, inject the `Injector` and/or the plugins we > are > >>> > >>> interested in. > >>> > >>> > >>> Thoughts? > >>> > >>> > >>> What you are suggesting is that the injector should just be another > bean > >>> that can be injected. I have no problem with that. > >>> > >>> I should mention that I asked Matt to look into an issue I have with > the > >> Spring > >>> support. Spring needs access to its Environment. We currently save that > >> in > >>> the LoggerContex. However, the SpringPropertySource has a circularity > >> problem. > >>> PropertySources are created before anything else in Log4j, including > the > >> Injector. > >>> The Spring support currently uses an EnvironmentHolder singleton. I’d > >> really like > >>> to do a PR for Spring to move the Spring Boot code there but I am not > >> comfortable > >>> doing it with the EnvironmentHolder. > >>> > >>> What happens how is that the EnvironmentHolder checks to see if Log4j > >> has initialized. > >>> If it hasn’t it returns null. If it has then it accesses the > >> LoggerContext to get the Environment. > >>> This just feels clunky. What I would prefer is to have the > >> PropertySource be injected > >>> with the Environment when it becomes available. In essence, deferred > >> injection. > >>> > >>> Ralph > >>> > >>> > >>> > >>> > >>> > >>> > >> >
