Re: 3.x plugins
> On May 30, 2022, at 2:26 PM, Matt Sicker wrote: > > When I was working on the previous iteration of the DI API, I had a > BeanProcessor which generated different metadata for different annotation > scenarios. It wasn’t too complex compared to the existing annotation > processor, though I ended up sticking with the existing processor for now > since that detail can be more flexibly changed over time. Perhaps the > PluginService API should at least abstract PluginEntry to an interface for > different plugin indexing scenarios. That could make it easier to index > additional metadata for different plugin types that we already have like > pattern converter keys. > — If you do that then you will somehow need to have metadata about each plugin type to know what field to use as an index. While that might be better in the long run it certainly won’t result in the plugin implementation being simpler. Ralph
Re: 3.x plugins
> On May 30, 2022, at 1:41 PM, Volkan Yazıcı wrote: > > On Mon, May 30, 2022 at 6:52 PM Ralph Goers > wrote: > >>> On May 30, 2022, at 2:01 AM, Volkan Yazıcı wrote: >>> 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? >> >> Why? What problem does this solve? >> >>> 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. >> >> This is a typical Plugin definition. >> >> @Plugin(name = "Failover", category = Core.CATEGORY_NAME, elementType = >> Appender.ELEMENT_TYPE, printObject = true) >> >> So you want to remove the elementType? Note that “name” is used as the >> key to store the plugin into its Category Map. The elementType is usually >> what is in the “name" attribute but some Plugins, such as Route, don’t >> have an elementType. In those cases the name attribute of the entry is set >> to the plugin’s name attribute. PluginRegistry creates a PluginType which >> includes the name attribute. This is used all over the place for various >> reasons. >> So I don’t see how either key or name can be removed since they are used >> for completely different purposes. >> > > I did not propose anywhere to make a change on or remove `elementType`, I > think we have a misunderstanding. My point is simple, `key` can be used > anywhere `name` is used right now. Am I mistaken with this assessment? Is > there any usage of `name` where if it is replaced by the contents of `key` > the functionality gets broken? Yes, you are mistaken in you assessment. I guess I must not have been clear in what I wrote above. For all practical purposes the “name” attribute and elementType are the same thing. Only when an element type is not declared does the “name” attribute have a different value. However, the key always has the value of the plugin name attribute. So no, they are not the same and no, one cannot be replaced with the other. > > >>> 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`. >> >> Maybe so, but Matt didn’t actually remove anything. IMO breaking up >> PluginEntry >> wouldn’t solve anything and would, in fact, make the code more complicated. >> > > I simply don't understand why we store every plugin as if it is something > to be read from a file-based configuration. If that is so, let's revert > Matt's changes and make sure every plugin must be configurable. If not, > let's reflect that in the code. `PluginEntry` shouldn't care about the > concerns of configurations. It is the configuration logic that needs to > figure out whether a particular `PluginEntry` implements > `ConfigurablePluginEntry` interface or not and acts accordingly. Only “Core” plugins are directly concerned about the Configuration. Admittedly, that is over 80% of the plugins. However, all the PatternConverters are driven by the pattern attribute wherever it is used. So that is also essentially a file-based configuration. We also have Lookups. But Lookups are located based on a Lookup pattern being specified somewhere in the configuration, so I guess they are file-based too. We also have Watcher plugins. Those are chosen based on the configuration file location String, so those are configuration based too. In short, the reason they are all tied to “file-based” configuration is because the whole point of plugins is to convert some textual configuration data into a set of methods that implement specific interfaces. Again, to me it feels like you are trying to “fix” something that isn’t broken. Ralph
Re: 3.x plugins
When I was working on the previous iteration of the DI API, I had a BeanProcessor which generated different metadata for different annotation scenarios. It wasn’t too complex compared to the existing annotation processor, though I ended up sticking with the existing processor for now since that detail can be more flexibly changed over time. Perhaps the PluginService API should at least abstract PluginEntry to an interface for different plugin indexing scenarios. That could make it easier to index additional metadata for different plugin types that we already have like pattern converter keys. — Matt Sicker > On May 30, 2022, at 15:41, Volkan Yazıcı wrote: > > On Mon, May 30, 2022 at 6:52 PM Ralph Goers > wrote: > >>> On May 30, 2022, at 2:01 AM, Volkan Yazıcı wrote: >>> 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? >> >> Why? What problem does this solve? >> >>> 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. >> >> This is a typical Plugin definition. >> >> @Plugin(name = "Failover", category = Core.CATEGORY_NAME, elementType = >> Appender.ELEMENT_TYPE, printObject = true) >> >> So you want to remove the elementType? Note that “name” is used as the >> key to store the plugin into its Category Map. The elementType is usually >> what is in the “name" attribute but some Plugins, such as Route, don’t >> have an elementType. In those cases the name attribute of the entry is set >> to the plugin’s name attribute. PluginRegistry creates a PluginType which >> includes the name attribute. This is used all over the place for various >> reasons. >> So I don’t see how either key or name can be removed since they are used >> for completely different purposes. >> > > I did not propose anywhere to make a change on or remove `elementType`, I > think we have a misunderstanding. My point is simple, `key` can be used > anywhere `name` is used right now. Am I mistaken with this assessment? Is > there any usage of `name` where if it is replaced by the contents of `key` > the functionality gets broken? > > >>> 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`. >> >> Maybe so, but Matt didn’t actually remove anything. IMO breaking up >> PluginEntry >> wouldn’t solve anything and would, in fact, make the code more complicated. >> > > I simply don't understand why we store every plugin as if it is something > to be read from a file-based configuration. If that is so, let's revert > Matt's changes and make sure every plugin must be configurable. If not, > let's reflect that in the code. `PluginEntry` shouldn't care about the > concerns of configurations. It is the configuration logic that needs to > figure out whether a particular `PluginEntry` implements > `ConfigurablePluginEntry` interface or not and acts accordingly.
Re: 3.x plugins
On Mon, May 30, 2022 at 6:52 PM Ralph Goers wrote: > > On May 30, 2022, at 2:01 AM, Volkan Yazıcı wrote: > > 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? > > Why? What problem does this solve? > > > 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. > > This is a typical Plugin definition. > > @Plugin(name = "Failover", category = Core.CATEGORY_NAME, elementType = > Appender.ELEMENT_TYPE, printObject = true) > > So you want to remove the elementType? Note that “name” is used as the > key to store the plugin into its Category Map. The elementType is usually > what is in the “name" attribute but some Plugins, such as Route, don’t > have an elementType. In those cases the name attribute of the entry is set > to the plugin’s name attribute. PluginRegistry creates a PluginType which > includes the name attribute. This is used all over the place for various > reasons. > So I don’t see how either key or name can be removed since they are used > for completely different purposes. > I did not propose anywhere to make a change on or remove `elementType`, I think we have a misunderstanding. My point is simple, `key` can be used anywhere `name` is used right now. Am I mistaken with this assessment? Is there any usage of `name` where if it is replaced by the contents of `key` the functionality gets broken? > > 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`. > > Maybe so, but Matt didn’t actually remove anything. IMO breaking up > PluginEntry > wouldn’t solve anything and would, in fact, make the code more complicated. > I simply don't understand why we store every plugin as if it is something to be read from a file-based configuration. If that is so, let's revert Matt's changes and make sure every plugin must be configurable. If not, let's reflect that in the code. `PluginEntry` shouldn't care about the concerns of configurations. It is the configuration logic that needs to figure out whether a particular `PluginEntry` implements `ConfigurablePluginEntry` interface or not and acts accordingly.
Re: Consuming our own BOM
And it’s much simpler than moving the BOM around. Very nice! — Matt Sicker > On May 30, 2022, at 09:44, Apache wrote: > > It is implemented on master. > > Ralph > >> On May 30, 2022, at 2:27 AM, Volkan Yazıcı wrote: >> >> Mind somebody sharing the last state? Is it implemented, if so how and on >> which branch(es)? Is it reverted? If so, totally or partially? >> >>> On Sun, May 29, 2022 at 9:53 AM Ralph Goers >>> wrote: >>> >>> That is OK. I have reverted your commit and am testing the build for a >>> second time doing it the correct way. >>> >>> Ralph >>> >> On May 28, 2022, at 9:14 PM, Matt Sicker wrote: > > It worked, but I had to specify where the parent pom was in the >>> submodules. Are you saying I could get the same effect by importing the bom >>> in the parent pom? If so, that certainly seems easier. — Matt Sicker > On May 28, 2022, at 18:15, Ralph Goers >>> wrote: > > Why is this necessary? I would think having the parent import the >>> bom/pom.xml should be enough. > > Ralph > >> On May 27, 2022, at 6:18 PM, Matt Sicker wrote: >> >> To avoid rearranging all the directories, I'm moving the parent pom to >> its own directory, moving the bom pom to the root, and updating the >> rest of the poms to know where the old parent pom now is. >> >>> On Thu, May 19, 2022 at 3:08 PM Matt Sicker wrote: >>> >>> Agreed. I added the BOM POM later on and didn’t know of any >>> established patterns for modules as BOMs weren’t used extensively quite yet >>> at the time (and it was a Maven specific feature then, too; Spring ported >>> the concept to Gradle later on, and now Gradle has a native concept of the >>> same thing). >>> >>> — >>> Matt Sicker >>> On May 19, 2022, at 10:33, Ralph Goers >>> wrote: >>> >>> Yes, that would make sense. I am sure this happened simply because >>> the bom pom.xml was introduced way after the first releases. >>> >>> Ralph >>> On May 18, 2022, at 11:38 PM, Volkan Yazıcı wrote: >>> >>> >>> Even though we provide a BOM module (`log4j-bom`), we don't consume it >>> >>> ourselves. Hence occasionally we end up publishing artifacts not >>> included >>> >>> in the BOM. Consuming our own BOM decreases the chances of missing out >>> >>> artifacts in BOM, though doesn't totally eliminate the chances of that >>> >>> happening. >>> >>> >>> When I read how Maven advises to structure the BOM module >>> >>> < >>> https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#bill-of-materials-bom-poms , >>> >>> I understand what needs to be in the case of Log4j is the following: >>> >>> >>> /pom.xml (`log4j-bom` module) >>> >>> /log4j-parent/pom.xml (`log4j` module importing `log4j-bom`) >>> >>> /log4j-parent/log4j-core/pom.xml (`log4j-core` module parented by >>> `log4j`) >>> >>> >>> Though what we have in reality is the following: >>> >>> >>> /log4j-bom/pom.xml (`log4j-bom` module) >>> >>> /pom.xml (`log4j` module parented by `logging-parent`) >>> >>> /log4j-core/pom.xml (`log4j-core` module parented by `log4j`) >>> >>> >>> Ideally we should follow the Maven-advised approach and consume from >>> our >>> >>> BOM parented by `logging-parent`. >>> >>> >>> What do you think? Is my interpretation of the Maven-advised approach >>> >>> correct? >>> >>> > >>> >>> >
Re: 3.x plugins
The annotations could index even more info if we wanted. ConverterKeys is another annotation that was introduced due to, uh, I don’t know why actually. Seems like it could use PluginAliases already to define extra converter keys. Non-core namespace plugins could potentially make use of some of the Configurable metadata, but we don’t have a current use for it, hence the split annotation. — Matt Sicker > On May 30, 2022, at 11:52, Ralph Goers wrote: > > > >>> On May 30, 2022, at 2:01 AM, Volkan Yazıcı wrote: >>> >>> 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? > > Why? What problem does this solve? > >> 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. > > This is a typical Plugin definition. > > @Plugin(name = "Failover", category = Core.CATEGORY_NAME, elementType = > Appender.ELEMENT_TYPE, printObject = true) > > So you want to remove the elementType? Note that “name” is used as the > key to store the plugin into its Category Map. The elementType is usually > what is in the “name" attribute but some Plugins, such as Route, don’t > have an elementType. In those cases the name attribute of the entry is set > to the plugin’s name attribute. PluginRegistry creates a PluginType which > includes the name attribute. This is used all over the place for various > reasons. > So I don’t see how either key or name can be removed since they are used > for completely different purposes. > >> 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`. > > Maybe so, but Matt didn’t actually remove anything. IMO breaking up > PluginEntry > wouldn’t solve anything and would, in fact, make the code more complicated. > > Ralph >
Re: 3.x plugins
> On May 30, 2022, at 2:01 AM, Volkan Yazıcı wrote: > >> 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? Why? What problem does this solve? > 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. This is a typical Plugin definition. @Plugin(name = "Failover", category = Core.CATEGORY_NAME, elementType = Appender.ELEMENT_TYPE, printObject = true) So you want to remove the elementType? Note that “name” is used as the key to store the plugin into its Category Map. The elementType is usually what is in the “name" attribute but some Plugins, such as Route, don’t have an elementType. In those cases the name attribute of the entry is set to the plugin’s name attribute. PluginRegistry creates a PluginType which includes the name attribute. This is used all over the place for various reasons. So I don’t see how either key or name can be removed since they are used for completely different purposes. > >>> 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`. Maybe so, but Matt didn’t actually remove anything. IMO breaking up PluginEntry wouldn’t solve anything and would, in fact, make the code more complicated. Ralph
Re: Consuming our own BOM
It is implemented on master. Ralph > On May 30, 2022, at 2:27 AM, Volkan Yazıcı wrote: > > Mind somebody sharing the last state? Is it implemented, if so how and on > which branch(es)? Is it reverted? If so, totally or partially? > >> On Sun, May 29, 2022 at 9:53 AM Ralph Goers >> wrote: >> >> That is OK. I have reverted your commit and am testing the build for a >> second time doing it the correct way. >> >> Ralph >> On May 28, 2022, at 9:14 PM, Matt Sicker wrote: >>> >>> It worked, but I had to specify where the parent pom was in the >> submodules. Are you saying I could get the same effect by importing the bom >> in the parent pom? If so, that certainly seems easier. >>> >>> — >>> Matt Sicker >>> On May 28, 2022, at 18:15, Ralph Goers >> wrote: Why is this necessary? I would think having the parent import the >> bom/pom.xml should be enough. Ralph > On May 27, 2022, at 6:18 PM, Matt Sicker wrote: > > To avoid rearranging all the directories, I'm moving the parent pom to > its own directory, moving the bom pom to the root, and updating the > rest of the poms to know where the old parent pom now is. > >> On Thu, May 19, 2022 at 3:08 PM Matt Sicker wrote: >> >> Agreed. I added the BOM POM later on and didn’t know of any >> established patterns for modules as BOMs weren’t used extensively quite yet >> at the time (and it was a Maven specific feature then, too; Spring ported >> the concept to Gradle later on, and now Gradle has a native concept of the >> same thing). >> >> — >> Matt Sicker >> >>> On May 19, 2022, at 10:33, Ralph Goers >> wrote: >> >> Yes, that would make sense. I am sure this happened simply because >> the bom pom.xml was introduced way after the first releases. >> >> Ralph >> >>> On May 18, 2022, at 11:38 PM, Volkan Yazıcı wrote: >> >> >> Even though we provide a BOM module (`log4j-bom`), we don't consume it >> >> ourselves. Hence occasionally we end up publishing artifacts not >> included >> >> in the BOM. Consuming our own BOM decreases the chances of missing out >> >> artifacts in BOM, though doesn't totally eliminate the chances of that >> >> happening. >> >> >> When I read how Maven advises to structure the BOM module >> >> < >> https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#bill-of-materials-bom-poms >>> , >> >> I understand what needs to be in the case of Log4j is the following: >> >> >> /pom.xml (`log4j-bom` module) >> >> /log4j-parent/pom.xml (`log4j` module importing `log4j-bom`) >> >> /log4j-parent/log4j-core/pom.xml (`log4j-core` module parented by >> `log4j`) >> >> >> Though what we have in reality is the following: >> >> >> /log4j-bom/pom.xml (`log4j-bom` module) >> >> /pom.xml (`log4j` module parented by `logging-parent`) >> >> /log4j-core/pom.xml (`log4j-core` module parented by `log4j`) >> >> >> Ideally we should follow the Maven-advised approach and consume from >> our >> >> BOM parented by `logging-parent`. >> >> >> What do you think? Is my interpretation of the Maven-advised approach >> >> correct? >> >> >> >>
Re: Release-2.x failure
It the db location could be changed to the target folder. Gary On Mon, May 30, 2022, 07:33 Gary Gregory wrote: > I've run into this before and yes the test should delete its database on > completion but obviously does not. > > Gary > > On Mon, May 30, 2022, 03:10 Piotr P. Karwasz > wrote: > >> On Mon, 30 May 2022 at 09:01, Ralph Goers >> wrote: >> >> > I always run mvn clean install. If there is something else that needs to >> > be cleaned up >> > the test should be taking care of it. >> > >> >> I totally agree, I'll find the tests that use H2 and fix them tonight. >> >> Piotr >> >
Re: Release-2.x failure
I've run into this before and yes the test should delete its database on completion but obviously does not. Gary On Mon, May 30, 2022, 03:10 Piotr P. Karwasz wrote: > On Mon, 30 May 2022 at 09:01, Ralph Goers > wrote: > > > I always run mvn clean install. If there is something else that needs to > > be cleaned up > > the test should be taking care of it. > > > > I totally agree, I'll find the tests that use H2 and fix them tonight. > > Piotr >
Re: Log4j 2.18.0
Hi Ralph, On Mon, 30 May 2022 at 09:11, Ralph Goers wrote: > I ran it multiple times in my Windows VM and it failed each time. I am not > going > to hunt down what directories I need to be deleting. The test needs to do > that. > I perfectly understand your POV. Actually looking at the test's code, it has some methods to clean up after himself, but they were not working. It should work now. I don't entirely understand why `JdbcAppenderColumnMappingLiteralTest` uses a file-backed DB, while the remaining tests use a memory-backed DB, but I left it the way it is. Piotr
Re: Consuming our own BOM
Mind somebody sharing the last state? Is it implemented, if so how and on which branch(es)? Is it reverted? If so, totally or partially? On Sun, May 29, 2022 at 9:53 AM Ralph Goers wrote: > That is OK. I have reverted your commit and am testing the build for a > second time doing it the correct way. > > Ralph > > > On May 28, 2022, at 9:14 PM, Matt Sicker wrote: > > > > It worked, but I had to specify where the parent pom was in the > submodules. Are you saying I could get the same effect by importing the bom > in the parent pom? If so, that certainly seems easier. > > > > — > > Matt Sicker > > > >> On May 28, 2022, at 18:15, Ralph Goers > wrote: > >> > >> Why is this necessary? I would think having the parent import the > bom/pom.xml should be enough. > >> > >> Ralph > >> > >>> On May 27, 2022, at 6:18 PM, Matt Sicker wrote: > >>> > >>> To avoid rearranging all the directories, I'm moving the parent pom to > >>> its own directory, moving the bom pom to the root, and updating the > >>> rest of the poms to know where the old parent pom now is. > >>> > On Thu, May 19, 2022 at 3:08 PM Matt Sicker wrote: > > Agreed. I added the BOM POM later on and didn’t know of any > established patterns for modules as BOMs weren’t used extensively quite yet > at the time (and it was a Maven specific feature then, too; Spring ported > the concept to Gradle later on, and now Gradle has a native concept of the > same thing). > > — > Matt Sicker > > > On May 19, 2022, at 10:33, Ralph Goers > wrote: > > Yes, that would make sense. I am sure this happened simply because > the bom pom.xml was introduced way after the first releases. > > Ralph > > > On May 18, 2022, at 11:38 PM, Volkan Yazıcı wrote: > > > Even though we provide a BOM module (`log4j-bom`), we don't consume it > > ourselves. Hence occasionally we end up publishing artifacts not > included > > in the BOM. Consuming our own BOM decreases the chances of missing out > > artifacts in BOM, though doesn't totally eliminate the chances of that > > happening. > > > When I read how Maven advises to structure the BOM module > > < > https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#bill-of-materials-bom-poms > >, > > I understand what needs to be in the case of Log4j is the following: > > > /pom.xml (`log4j-bom` module) > > /log4j-parent/pom.xml (`log4j` module importing `log4j-bom`) > > /log4j-parent/log4j-core/pom.xml (`log4j-core` module parented by > `log4j`) > > > Though what we have in reality is the following: > > > /log4j-bom/pom.xml (`log4j-bom` module) > > /pom.xml (`log4j` module parented by `logging-parent`) > > /log4j-core/pom.xml (`log4j-core` module parented by `log4j`) > > > Ideally we should follow the Maven-advised approach and consume from > our > > BOM parented by `logging-parent`. > > > What do you think? Is my interpretation of the Maven-advised approach > > correct? > > > >> > >
Re: 3.x plugins
> 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 wrote:
> And now that I just started working on Map> 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 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
> wrote:
> >>
> >>
> >>
> >>> On May 27, 2022, at 6:54 AM, Volkan Yazıcı 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());
> >>
Re: Is there a way for Dependabot to use its own fork or something?
Matt, mind elaborating a bit on what exactly is the problem and how will a fork fix that? On Sat, May 28, 2022 at 2:27 AM Matt Sicker wrote: > The fact that the bot uses branches in our repo rather than a fork of > the repo causes a shitload of bot spam. >
Re: Log4j 2.18.0
I ran it multiple times in my Windows VM and it failed each time. I am not going to hunt down what directories I need to be deleting. The test needs to do that. Since H2 is only used for testing I don’t consider it a big deal. The link you provided clearly shows it is a test dependency so almost no-one will care. But if you want to fix the failing test I am OK with that. I should point out though that this is the poster boy for why we need to break up Log4j into multiple repos. Or rather, LOG4J2-3428 makes it clear why. It shows 36 dependencies being upgraded since 2.17.2 was released. I am betting most of them are test dependencies but still, with that many dependencies updated the likelihood of problems goes way up. Ralph > On May 29, 2022, at 11:54 PM, Piotr P. Karwasz > wrote: > > Hi Ralph, > > On Mon, 30 May 2022 at 00:26, Ralph Goers > wrote: > >> FWIW, the h2 upgrade broke the build. I am in the process of reverting the >> version change. >> >> Ralph >> > > I would really like the H2 upgrade to be included in 2.18.0, so that pages > like this: > > https://mvnrepository.com/artifact/org.apache.logging.log4j/log4j-core/2.17.2 > > don't have any red notices. > > As I explained in another e-mail, the failure of the test is due to the > fact that the test leaves garbage in the temporary directory. When you > switch from H2 1.x to H2 2.x the test tries to load the leftovers of the > previous execution of the test and fails. > > After 2.18.0 I can find all tests to pinpoint those that leave files in the > filesystem. Since Surefire runs each test in a separate JVM, I believe that > by correcting the misbehaving tests, we will be able to run tests in > parallel. > > Piotr
Re: Release-2.x failure
On Mon, 30 May 2022 at 09:01, Ralph Goers wrote: > I always run mvn clean install. If there is something else that needs to > be cleaned up > the test should be taking care of it. > I totally agree, I'll find the tests that use H2 and fix them tonight. Piotr
Re: Release-2.x failure
> On May 29, 2022, at 9:12 PM, Piotr P. Karwasz wrote: > > Hi Ralph, > > On Mon, 30 May 2022 at 00:14, Ralph Goers > wrote: > >> Running the test individually shows >> >> Caused by: org.h2.mvstore.MVStoreException: The write format 1 is smaller >> than the supported format 2 [2.1.212/5] >> >> So the h2 dependency upgrade apparently wasn’t tested. >> > > You need to clean the temporary directory on the machine running the tests: > some H2 tests write a temporary database there and fail just after the > upgrade. > I always run mvn clean install. If there is something else that needs to be cleaned up the test should be taking care of it. Ralph
Re: Log4j 2.18.0
Hi Ralph, On Mon, 30 May 2022 at 00:26, Ralph Goers wrote: > FWIW, the h2 upgrade broke the build. I am in the process of reverting the > version change. > > Ralph > I would really like the H2 upgrade to be included in 2.18.0, so that pages like this: https://mvnrepository.com/artifact/org.apache.logging.log4j/log4j-core/2.17.2 don't have any red notices. As I explained in another e-mail, the failure of the test is due to the fact that the test leaves garbage in the temporary directory. When you switch from H2 1.x to H2 2.x the test tries to load the leftovers of the previous execution of the test and fails. After 2.18.0 I can find all tests to pinpoint those that leave files in the filesystem. Since Surefire runs each test in a separate JVM, I believe that by correcting the misbehaving tests, we will be able to run tests in parallel. Piotr
