I assume all of this stuff doesn’t break compatibility with 2.x plugins? Ralph
> On May 20, 2022, at 2:36 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 >> >> >> >> >> >>
