Having to make changes when recompiling is fine. Ralph
> On May 20, 2022, at 4:14 PM, Matt Sicker <[email protected]> wrote: > > Not beyond the fact that @Plugin moved packages and already breaks > recompiling plugins from 2.x to 3.x. However, the metadata is loaded from 2.x > plugins via the .dat files, and that still loads the metadata appropriately. > There’s at least one test in JsonTemplateLayout for comparing behavior to an > old version of the plugin from before it was added to the project. That’s > also where I verified that the missing log4j-core version of @Plugin doesn’t > prevent old plugins from still working (it’s only recompiling them against > 3.x that requires modifications on the annotation imports and which > annotations to use in general here, though the rest of the plugin annotations > still have legacy copies in log4j-core). > — > Matt Sicker > >> On May 20, 2022, at 17:23, Ralph Goers <[email protected]> wrote: >> >> 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 >>>> >>>> >>>> >>>> >>>> >>>> >> >
