And now that I just started working on Map<String, Supplier<T>> injection, I
see where this could would have otherwise been used. When streaming through
plugins, right now, all plugin classes in that namespace get loaded to at least
check their ordering annotations (another bit of metadata that could be indexed
now) besides checking their type assignability. For the more common case where
we can index both the ordering and the implemented interfaces, we can more
efficiently stream and inject plugin factories without loading the plugin class
until said plugin needs to be created in the first place. I think I’ll re-add
these bits of metadata to maintain the lazy loading aspect.
—
Matt Sicker
> On May 27, 2022, at 11:08, Matt Sicker <boa...@gmail.com> wrote:
>
> 1. Plugin key is separate from the plugin name due to automatically
> filling in some info from elementType, though I've rearranged the code
> a bit in 3.x to make this part less confusing.
>
> 2. It's only @Configurable (Core) plugins that use the element type,
> printable, and deferChildren options. I could potentially break up the
> classes, but that does complicate other areas like annotation
> processing.
>
> 3. This is somewhat of a relic from attempting to keep plugin loading
> lazy. The idea here was that we could find plugins by their
> implemented interface as a way to collect a subset of them without
> loading their classes, but this still hasn't replaced the use of
> elementType or namespace that already implicitly organize plugins by
> interface. I'll likely remove this.
>
> 4. Yeah, there's surface-level overlap, but they have opposite
> purposes: constraint validators will prevent invalid objects from
> being created, while conditionals will allow certain objects to be
> created based on some state. One is for raising errors, the other is
> for conditionally enabling things.
>
> 5 and 6. I'd be open to further organizing in the log4j-plugins module.
>
> On Fri, May 27, 2022 at 10:38 AM Ralph Goers <ralph.go...@dslextreme.com>
> wrote:
>>
>>
>>
>>> On May 27, 2022, at 6:54 AM, Volkan Yazıcı <vol...@yazi.ci> wrote:
>>>
>>> Matt, some more questions/remarks:
>>>
>>> 1. AFAIU, plugin `key` is the normalized `name`. Right? If so, what is
>>> the purpose of the `name`?
>>
>> See PluginElementVisitor (at least in 2.x)
>>
>> @Override
>> public PluginEntry visitType(final TypeElement e, final Plugin plugin) {
>> Objects.requireNonNull(plugin, "Plugin annotation is null.");
>> final PluginEntry entry = new PluginEntry();
>> entry.setKey(plugin.name().toLowerCase(Locale.US));
>> entry.setClassName(elements.getBinaryName(e).toString());
>> entry.setName(Plugin.EMPTY.equals(plugin.elementType()) ? plugin.name() :
>> plugin.elementType());
>> entry.setPrintable(plugin.printObject());
>> entry.setDefer(plugin.deferChildren());
>> entry.setCategory(plugin.category());
>> return entry;
>> }
>> Some elements (such as KeyValuePair) don’t specify an elementType. In that
>> case the name attribute is set to the plugin’s name attribute value.
>> Otherwise
>> it is the elementType. The key is always the Plugin’s name attribute.
>>
>>
>>
>>> 2. I was expecting `PluginEntry` to be simplified similar to `Plugin`
>>> and only contain a `String key`, `String name`, and `String className`.
>>
>> Why would you expect that? All the fields are used by the Plugin system and
>> need to be in the plugin metadata to load it. Remember, at this point we
>> don’t
>> use the annotations any more.
>>
>>
>>> For
>>> `@Configurable`, we could create a `ConfigurablePlugin` class extending
>>> from `Plugin` with extra `elementType`, `printable`, etc. fields. Generated
>>> `Log4jPlugins.java` could instantiate either `Plugin` or
>>> `ConfigurablePlugin` depending on the need. In its current state, `Plugin`
>>> looks pretty generic, though the rest still resembles the past
>>> architecture.
>>
>> IMO what you propose just makes things unnecessarily complicated.
>>
>>> 3. What is the purpose of populating interfaces (i.e.,
>>> `PluginEntry#interfaces`) at compile-time? What does use them and how?
>>
>> I’m interested in this too as PluginProcessor doesn’t seem to be setting any
>> values.
>>
>>> 4. I see quite some overlap between `plugins.condition` and
>>> `plugins.validation`. I would prefer the latter to be modernized and merged
>>> into the former.
>>> 5. I would prefer `Configurable`, `Node`, et al. to be moved to their
>>> own `plugins.config` package.
>>
>> Not sure I agree with this.
>>
>>> 6. I would prefer `di.Scope` et al. to be moved to their own
>>> `plugins.scope` package – or at least, be located in the same package. For
>>> instance, consider `plugins.di.Scope` vs. `plugins.ScopeType`.
>>
>> I agree it should be moved. It could be either plugins.scope or
>> plugins.di.scope.
>> I’d prefer the former since it is related to the Plugin annotation
>>
>> Ralph
