> plugin entry interfaces
As Matt has indicated, we indeed need to keep `PluginEntry#interfaces` for
lazy loading – plus a new `PluginEntry#order` field.
> plugin name vs key
Following up on the responses, I will ask another question: In 3.x, can't
we deprecate either key or name and stick to only one? For instance, I am
in favor of keeping only `name`, adding a compile-time check to validate
its content, and cloning it in a `key` field (marked as to-be-removed in
future) for backward compatibility. The bottom line is, I think if we _can_
live with one, we should.
> > 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.
Yes, but it completes an important refactoring Matt has been tackling:
plugin simplification. The frontend (user-facing) part of this story is
implemented; `@Configurable` and `@Plugin` are two separate concerns.
Though the backend is still the same: the entire `@Plugin` metadata model
contains properties of `@Configurable`.
> 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.
Check.
On Sat, May 28, 2022 at 10:06 PM Matt Sicker <boa...@gmail.com> wrote:
> 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
>
>
