You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@logging.apache.org by Ralph Goers <ra...@dslextreme.com> on 2021/04/03 20:27:41 UTC
Re: [logging-log4j2] branch release-2.x updated: LOG4J2-3004 Add
plugin support to JsonTemplateLayout. (#476)
Volkan,
I haven’t looked at the details of what you changed in JsonTemplateLayout yet but the below concerns me a bit….
Ralph
> On Apr 3, 2021, at 3:48 AM, vy@apache.org wrote:
>
> This is an automated email from the ASF dual-hosted git repository.
>
Why were changes to the registry required for this? Aren’t you just creating new Plugins?
> diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> index 5088f15..3964370 100644
> --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> @@ -86,8 +86,9 @@ public class TypeConverterRegistry {
> if (clazz.isEnum()) {
> @SuppressWarnings({"unchecked","rawtypes"})
> final EnumConverter<? extends Enum> converter = new EnumConverter(clazz.asSubclass(Enum.class));
I don’t understand how replacing calling registerConverter with putIfAbsent is equivalent. The registerConverterMethod seems to allow plugins to be overridden.
> - registry.putIfAbsent(type, converter);
> - return converter;
> + synchronized (INSTANCE_LOCK) {
> + return registerConverter(type, converter);
> + }
> }
> }
> // look for compatible converters
> @@ -96,8 +97,9 @@ public class TypeConverterRegistry {
> if (TypeUtil.isAssignable(type, key)) {
> LOGGER.debug("Found compatible TypeConverter<{}> for type [{}].", key, type);
> final TypeConverter<?> value = entry.getValue();
> - registry.putIfAbsent(type, value);
> - return value;
> + synchronized (INSTANCE_LOCK) {
> + return registerConverter(type, value);
> + }
> }
> }
> throw new UnknownFormatConversionException(type.toString());
> @@ -119,11 +121,52 @@ public class TypeConverterRegistry {
> final Class<? extends TypeConverter> pluginClass = clazz.asSubclass(TypeConverter.class);
> final Type conversionType = getTypeConverterSupportedType(pluginClass);
> final TypeConverter<?> converter = ReflectionUtil.instantiate(pluginClass);
> - if (registry.putIfAbsent(conversionType, converter) != null) {
> - LOGGER.warn("Found a TypeConverter [{}] for type [{}] that already exists.", converter,
> - conversionType);
> - }
> + registerConverter(conversionType, converter);
> + }
> + }
> + }
> +
> + /**
> + * Attempts to register the given converter and returns the effective
> + * converter associated with the given type.
> + * <p>
> + * Registration will fail if there already exists a converter for the given
> + * type and neither the existing, nor the provided converter extends from {@link Comparable}.
> + */
> + private TypeConverter<?> registerConverter(
> + final Type conversionType,
> + final TypeConverter<?> converter) {
> + final TypeConverter<?> conflictingConverter = registry.get(conversionType);
> + if (conflictingConverter != null) {
> + final boolean overridable;
> + if (converter instanceof Comparable) {
> + @SuppressWarnings("unchecked")
> + final Comparable<TypeConverter<?>> comparableConverter =
> + (Comparable<TypeConverter<?>>) converter;
> + overridable = comparableConverter.compareTo(conflictingConverter) < 0;
> + } else if (conflictingConverter instanceof Comparable) {
> + @SuppressWarnings("unchecked")
> + final Comparable<TypeConverter<?>> comparableConflictingConverter =
> + (Comparable<TypeConverter<?>>) conflictingConverter;
> + overridable = comparableConflictingConverter.compareTo(converter) > 0;
> + } else {
> + overridable = false;
> + }
> + if (overridable) {
> + LOGGER.debug(
> + "Replacing TypeConverter [{}] for type [{}] with [{}] after comparison.",
> + conflictingConverter, conversionType, converter);
> + registry.put(conversionType, converter);
> + return converter;
> + } else {
> + LOGGER.warn(
> + "Ignoring TypeConverter [{}] for type [{}] that conflicts with [{}], since they are not comparable.",
> + converter, conversionType, conflictingConverter);
> + return conflictingConverter;
> }
> + } else {
> + registry.put(conversionType, converter);
> + return converter;
> }
> }
Re: [logging-log4j2] branch release-2.x updated: LOG4J2-3004 Add
plugin support to JsonTemplateLayout. (#476)
Posted by Matt Sicker <bo...@gmail.com>.
Sounds like annotation processing has managed to get disabled in your
IDE. IDEs tend to disable that by default due to the ability to run
arbitrary code in annotation processors (but now IntelliJ has a
"trusted project" feature for its own metadata, so maybe that's
changed). Without the processor, you'll usually see errors like an
inability to find any plugins or null pointer exceptions.
On Tue, 6 Apr 2021 at 15:27, Volkan Yazıcı <vo...@gmail.com> wrote:
>
> Ralph, same here. I had tested it before pushing the commit. Now I cannot
> make it work either. JTL cannot find any of its plugins. Even reverting
> back to the point where I committed doesn't help either. I also cannot run
> tests in the IDE anymore, hence it is pretty difficult to debug for me. (I
> have shared this problem in another thread for future reference.)
>
> Please note that what broke release-2.x branch wasn't new functionality,
> but new dummy TypeConverter classes in TypeConverterRegistryTest. I have
> the feeling that there is something else wrong in the core and these new
> test classes just make it surface. @Matt, given you are the plugin expert,
> I presume, do you have any ideas?
>
> On Tue, Apr 6, 2021 at 6:56 AM Ralph Goers <ra...@dslextreme.com>
> wrote:
>
> > OK, release-2.x now seems to be better but I don’t understand what is
> > going on with Master. I am sure I built master several times over the
> > weekend. But now it is getting errors all over the place in Json Template
> > Layout tests. I did two commits that shouldn’t have impacted anything in
> > that.
> >
> > My only concern when code like the registry changes is that all the
> > existing unit tests continue to pass without having to change them (unless
> > they are clearly buggy). We have enough tests that I am pretty confident
> > that if they all pass nothing that is going to affect anything has changed.
> >
> > Ralph
> >
> > > On Apr 5, 2021, at 1:19 PM, Volkan Yazıcı <vo...@gmail.com>
> > wrote:
> > >
> > > Hello Ralph,
> > >
> > > *TypeConverterRegistry changes*
> > >
> > > Sorry for breaking certain tests. I had used the following to test my
> > > changes: `./mvnw clean install -pl
> > > :log4j-core,:log4j-layout-template-json`. I have tested my changes after
> > > purging Log4j `2.*-SNAPSHOT` artifacts from my `~/.m2` and there I indeed
> > > observe the same errors reported in Jenkins:
> > >
> > > org.apache.logging.log4j.core.config.LoggersPluginTest.testEmptyAttribute
> > >
> > org.apache.logging.log4j.core.config.plugins.validation.validators.ValidatingPluginWithFailoverTest.testDoesNotLog_NoParameterThatMatchesElement_message
> > >
> > > (Apparently I have needed an extra bootstrap for annotations to kick in.)
> > >
> > > Though something interesting is going on here. If I revert my changes to
> > > TypeConverterRegistry, *aforementioned tests still do fail*! It is
> > actually
> > > TypeConverterRegistryTest changes that is breaking those changes. I have
> > > experimented with the following combinations:
> > >
> > > - reverted TypeConverterRegistry ⇨ LoggersPluginTest fails
> > > - reverted TypeConverterRegistryTest ⇨ LoggersPluginTest succeeds
> > > - `@Ignore`d added tests to TypeConverterRegistryTest ⇨ LoggersPluginTest
> > > fails
> > >
> > > I have reverted the changes to TypeConverterRegistryTest, though the
> > > TypeConverterRegistry changes are still in. I hope this would suffice to
> > > make Jenkins and GitHub Actions happy.
> > >
> > > *RecyclerFactory customization support*
> > >
> > > In JsonTemplateLayout.Builder, I have a @PluginBuilderAttribute of type
> > > RecyclerFactory. The input string is converted to a RecyclerFactory using
> > > RecyclerFactoryConverter. To allow users to introduce their own
> > > RecyclerFactory implementation in a backward-compatible way, I thought of
> > > supporting multiple TypeConverter's in TypeConverterRegistry. The idea is
> > > to enhance TypeConverterRegistry such that in the presence of multiple
> > > TypeConverter's matching on the same footprint, TypeConverterRegistry
> > will
> > > use the Comparable interface to determine the effective one.
> > >
> > > I think it is a nice feature to support multiple TypeConverter's, as my
> > use
> > > case demonstrates. Do you agree? What else would you recommend me to
> > > implement this customization support needed for
> > > JsonTemplateLayout.Builder#recyclerFactory?
> > >
> > > Kind regards.
> > >
> > > On Sun, Apr 4, 2021 at 8:43 AM Ralph Goers <ra...@dslextreme.com>
> > > wrote:
> > >
> > >> I am now looking at the Jenkins and GitHub Actions builds and it is
> > clear
> > >> that the change I highlighted below has caused at 3 tests to fail.
> > Please
> > >> revert the change to the TypeConverterRegistry. In the future, whenever
> > a
> > >> change is made to the API or core components please make sure you run a
> > >> full build before you commit. This should be standard practice for
> > everyone.
> > >>
> > >> Ralph
> > >>
> > >>> On Apr 3, 2021, at 1:27 PM, Ralph Goers <ra...@dslextreme.com>
> > >> wrote:
> > >>>
> > >>> Volkan,
> > >>>
> > >>> I haven’t looked at the details of what you changed in
> > >> JsonTemplateLayout yet but the below concerns me a bit….
> > >>>
> > >>> Ralph
> > >>>
> > >>>
> > >>>> On Apr 3, 2021, at 3:48 AM, vy@apache.org wrote:
> > >>>>
> > >>>> This is an automated email from the ASF dual-hosted git repository.
> > >>>>
> > >>>
> > >>> Why were changes to the registry required for this? Aren’t you just
> > >> creating new Plugins?
> > >>>
> > >>>> diff --git
> > >>
> > a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> > >>
> > b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> > >>>> index 5088f15..3964370 100644
> > >>>> ---
> > >>
> > a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> > >>>> +++
> > >>
> > b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> > >>>> @@ -86,8 +86,9 @@ public class TypeConverterRegistry {
> > >>>> if (clazz.isEnum()) {
> > >>>> @SuppressWarnings({"unchecked","rawtypes"})
> > >>>> final EnumConverter<? extends Enum> converter = new
> > >> EnumConverter(clazz.asSubclass(Enum.class));
> > >>>
> > >>> I don’t understand how replacing calling registerConverter with
> > >> putIfAbsent is equivalent. The registerConverterMethod seems to allow
> > >> plugins to be overridden.
> > >>>
> > >>>> - registry.putIfAbsent(type, converter);
> > >>>> - return converter;
> > >>>> + synchronized (INSTANCE_LOCK) {
> > >>>> + return registerConverter(type, converter);
> > >>>> + }
> > >>>> }
> > >>>> }
> > >>>> // look for compatible converters
> > >>>> @@ -96,8 +97,9 @@ public class TypeConverterRegistry {
> > >>>> if (TypeUtil.isAssignable(type, key)) {
> > >>>> LOGGER.debug("Found compatible TypeConverter<{}> for
> > >> type [{}].", key, type);
> > >>>> final TypeConverter<?> value = entry.getValue();
> > >>>> - registry.putIfAbsent(type, value);
> > >>>> - return value;
> > >>>> + synchronized (INSTANCE_LOCK) {
> > >>>> + return registerConverter(type, value);
> > >>>> + }
> > >>>> }
> > >>>> }
> > >>>> throw new UnknownFormatConversionException(type.toString());
> > >>>> @@ -119,11 +121,52 @@ public class TypeConverterRegistry {
> > >>>> final Class<? extends TypeConverter> pluginClass =
> > >> clazz.asSubclass(TypeConverter.class);
> > >>>> final Type conversionType =
> > >> getTypeConverterSupportedType(pluginClass);
> > >>>> final TypeConverter<?> converter =
> > >> ReflectionUtil.instantiate(pluginClass);
> > >>>> - if (registry.putIfAbsent(conversionType, converter)
> > !=
> > >> null) {
> > >>>> - LOGGER.warn("Found a TypeConverter [{}] for type
> > >> [{}] that already exists.", converter,
> > >>>> - conversionType);
> > >>>> - }
> > >>>> + registerConverter(conversionType, converter);
> > >>>> + }
> > >>>> + }
> > >>>> + }
> > >>>> +
> > >>>> + /**
> > >>>> + * Attempts to register the given converter and returns the
> > >> effective
> > >>>> + * converter associated with the given type.
> > >>>> + * <p>
> > >>>> + * Registration will fail if there already exists a converter for
> > >> the given
> > >>>> + * type and neither the existing, nor the provided converter
> > >> extends from {@link Comparable}.
> > >>>> + */
> > >>>> + private TypeConverter<?> registerConverter(
> > >>>> + final Type conversionType,
> > >>>> + final TypeConverter<?> converter) {
> > >>>> + final TypeConverter<?> conflictingConverter =
> > >> registry.get(conversionType);
> > >>>> + if (conflictingConverter != null) {
> > >>>> + final boolean overridable;
> > >>>> + if (converter instanceof Comparable) {
> > >>>> + @SuppressWarnings("unchecked")
> > >>>> + final Comparable<TypeConverter<?>>
> > comparableConverter
> > >> =
> > >>>> + (Comparable<TypeConverter<?>>) converter;
> > >>>> + overridable =
> > >> comparableConverter.compareTo(conflictingConverter) < 0;
> > >>>> + } else if (conflictingConverter instanceof Comparable) {
> > >>>> + @SuppressWarnings("unchecked")
> > >>>> + final Comparable<TypeConverter<?>>
> > >> comparableConflictingConverter =
> > >>>> + (Comparable<TypeConverter<?>>)
> > >> conflictingConverter;
> > >>>> + overridable =
> > >> comparableConflictingConverter.compareTo(converter) > 0;
> > >>>> + } else {
> > >>>> + overridable = false;
> > >>>> + }
> > >>>> + if (overridable) {
> > >>>> + LOGGER.debug(
> > >>>> + "Replacing TypeConverter [{}] for type [{}]
> > >> with [{}] after comparison.",
> > >>>> + conflictingConverter, conversionType,
> > >> converter);
> > >>>> + registry.put(conversionType, converter);
> > >>>> + return converter;
> > >>>> + } else {
> > >>>> + LOGGER.warn(
> > >>>> + "Ignoring TypeConverter [{}] for type [{}]
> > >> that conflicts with [{}], since they are not comparable.",
> > >>>> + converter, conversionType,
> > >> conflictingConverter);
> > >>>> + return conflictingConverter;
> > >>>> }
> > >>>> + } else {
> > >>>> + registry.put(conversionType, converter);
> > >>>> + return converter;
> > >>>> }
> > >>>> }
> > >>>
> > >>>
> > >>>
> > >>
> > >>
> > >>
> >
> >
> >
Re: [logging-log4j2] branch release-2.x updated: LOG4J2-3004 Add
plugin support to JsonTemplateLayout. (#476)
Posted by Volkan Yazıcı <vo...@gmail.com>.
Ralph, same here. I had tested it before pushing the commit. Now I cannot
make it work either. JTL cannot find any of its plugins. Even reverting
back to the point where I committed doesn't help either. I also cannot run
tests in the IDE anymore, hence it is pretty difficult to debug for me. (I
have shared this problem in another thread for future reference.)
Please note that what broke release-2.x branch wasn't new functionality,
but new dummy TypeConverter classes in TypeConverterRegistryTest. I have
the feeling that there is something else wrong in the core and these new
test classes just make it surface. @Matt, given you are the plugin expert,
I presume, do you have any ideas?
On Tue, Apr 6, 2021 at 6:56 AM Ralph Goers <ra...@dslextreme.com>
wrote:
> OK, release-2.x now seems to be better but I don’t understand what is
> going on with Master. I am sure I built master several times over the
> weekend. But now it is getting errors all over the place in Json Template
> Layout tests. I did two commits that shouldn’t have impacted anything in
> that.
>
> My only concern when code like the registry changes is that all the
> existing unit tests continue to pass without having to change them (unless
> they are clearly buggy). We have enough tests that I am pretty confident
> that if they all pass nothing that is going to affect anything has changed.
>
> Ralph
>
> > On Apr 5, 2021, at 1:19 PM, Volkan Yazıcı <vo...@gmail.com>
> wrote:
> >
> > Hello Ralph,
> >
> > *TypeConverterRegistry changes*
> >
> > Sorry for breaking certain tests. I had used the following to test my
> > changes: `./mvnw clean install -pl
> > :log4j-core,:log4j-layout-template-json`. I have tested my changes after
> > purging Log4j `2.*-SNAPSHOT` artifacts from my `~/.m2` and there I indeed
> > observe the same errors reported in Jenkins:
> >
> > org.apache.logging.log4j.core.config.LoggersPluginTest.testEmptyAttribute
> >
> org.apache.logging.log4j.core.config.plugins.validation.validators.ValidatingPluginWithFailoverTest.testDoesNotLog_NoParameterThatMatchesElement_message
> >
> > (Apparently I have needed an extra bootstrap for annotations to kick in.)
> >
> > Though something interesting is going on here. If I revert my changes to
> > TypeConverterRegistry, *aforementioned tests still do fail*! It is
> actually
> > TypeConverterRegistryTest changes that is breaking those changes. I have
> > experimented with the following combinations:
> >
> > - reverted TypeConverterRegistry ⇨ LoggersPluginTest fails
> > - reverted TypeConverterRegistryTest ⇨ LoggersPluginTest succeeds
> > - `@Ignore`d added tests to TypeConverterRegistryTest ⇨ LoggersPluginTest
> > fails
> >
> > I have reverted the changes to TypeConverterRegistryTest, though the
> > TypeConverterRegistry changes are still in. I hope this would suffice to
> > make Jenkins and GitHub Actions happy.
> >
> > *RecyclerFactory customization support*
> >
> > In JsonTemplateLayout.Builder, I have a @PluginBuilderAttribute of type
> > RecyclerFactory. The input string is converted to a RecyclerFactory using
> > RecyclerFactoryConverter. To allow users to introduce their own
> > RecyclerFactory implementation in a backward-compatible way, I thought of
> > supporting multiple TypeConverter's in TypeConverterRegistry. The idea is
> > to enhance TypeConverterRegistry such that in the presence of multiple
> > TypeConverter's matching on the same footprint, TypeConverterRegistry
> will
> > use the Comparable interface to determine the effective one.
> >
> > I think it is a nice feature to support multiple TypeConverter's, as my
> use
> > case demonstrates. Do you agree? What else would you recommend me to
> > implement this customization support needed for
> > JsonTemplateLayout.Builder#recyclerFactory?
> >
> > Kind regards.
> >
> > On Sun, Apr 4, 2021 at 8:43 AM Ralph Goers <ra...@dslextreme.com>
> > wrote:
> >
> >> I am now looking at the Jenkins and GitHub Actions builds and it is
> clear
> >> that the change I highlighted below has caused at 3 tests to fail.
> Please
> >> revert the change to the TypeConverterRegistry. In the future, whenever
> a
> >> change is made to the API or core components please make sure you run a
> >> full build before you commit. This should be standard practice for
> everyone.
> >>
> >> Ralph
> >>
> >>> On Apr 3, 2021, at 1:27 PM, Ralph Goers <ra...@dslextreme.com>
> >> wrote:
> >>>
> >>> Volkan,
> >>>
> >>> I haven’t looked at the details of what you changed in
> >> JsonTemplateLayout yet but the below concerns me a bit….
> >>>
> >>> Ralph
> >>>
> >>>
> >>>> On Apr 3, 2021, at 3:48 AM, vy@apache.org wrote:
> >>>>
> >>>> This is an automated email from the ASF dual-hosted git repository.
> >>>>
> >>>
> >>> Why were changes to the registry required for this? Aren’t you just
> >> creating new Plugins?
> >>>
> >>>> diff --git
> >>
> a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> >>
> b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> >>>> index 5088f15..3964370 100644
> >>>> ---
> >>
> a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> >>>> +++
> >>
> b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> >>>> @@ -86,8 +86,9 @@ public class TypeConverterRegistry {
> >>>> if (clazz.isEnum()) {
> >>>> @SuppressWarnings({"unchecked","rawtypes"})
> >>>> final EnumConverter<? extends Enum> converter = new
> >> EnumConverter(clazz.asSubclass(Enum.class));
> >>>
> >>> I don’t understand how replacing calling registerConverter with
> >> putIfAbsent is equivalent. The registerConverterMethod seems to allow
> >> plugins to be overridden.
> >>>
> >>>> - registry.putIfAbsent(type, converter);
> >>>> - return converter;
> >>>> + synchronized (INSTANCE_LOCK) {
> >>>> + return registerConverter(type, converter);
> >>>> + }
> >>>> }
> >>>> }
> >>>> // look for compatible converters
> >>>> @@ -96,8 +97,9 @@ public class TypeConverterRegistry {
> >>>> if (TypeUtil.isAssignable(type, key)) {
> >>>> LOGGER.debug("Found compatible TypeConverter<{}> for
> >> type [{}].", key, type);
> >>>> final TypeConverter<?> value = entry.getValue();
> >>>> - registry.putIfAbsent(type, value);
> >>>> - return value;
> >>>> + synchronized (INSTANCE_LOCK) {
> >>>> + return registerConverter(type, value);
> >>>> + }
> >>>> }
> >>>> }
> >>>> throw new UnknownFormatConversionException(type.toString());
> >>>> @@ -119,11 +121,52 @@ public class TypeConverterRegistry {
> >>>> final Class<? extends TypeConverter> pluginClass =
> >> clazz.asSubclass(TypeConverter.class);
> >>>> final Type conversionType =
> >> getTypeConverterSupportedType(pluginClass);
> >>>> final TypeConverter<?> converter =
> >> ReflectionUtil.instantiate(pluginClass);
> >>>> - if (registry.putIfAbsent(conversionType, converter)
> !=
> >> null) {
> >>>> - LOGGER.warn("Found a TypeConverter [{}] for type
> >> [{}] that already exists.", converter,
> >>>> - conversionType);
> >>>> - }
> >>>> + registerConverter(conversionType, converter);
> >>>> + }
> >>>> + }
> >>>> + }
> >>>> +
> >>>> + /**
> >>>> + * Attempts to register the given converter and returns the
> >> effective
> >>>> + * converter associated with the given type.
> >>>> + * <p>
> >>>> + * Registration will fail if there already exists a converter for
> >> the given
> >>>> + * type and neither the existing, nor the provided converter
> >> extends from {@link Comparable}.
> >>>> + */
> >>>> + private TypeConverter<?> registerConverter(
> >>>> + final Type conversionType,
> >>>> + final TypeConverter<?> converter) {
> >>>> + final TypeConverter<?> conflictingConverter =
> >> registry.get(conversionType);
> >>>> + if (conflictingConverter != null) {
> >>>> + final boolean overridable;
> >>>> + if (converter instanceof Comparable) {
> >>>> + @SuppressWarnings("unchecked")
> >>>> + final Comparable<TypeConverter<?>>
> comparableConverter
> >> =
> >>>> + (Comparable<TypeConverter<?>>) converter;
> >>>> + overridable =
> >> comparableConverter.compareTo(conflictingConverter) < 0;
> >>>> + } else if (conflictingConverter instanceof Comparable) {
> >>>> + @SuppressWarnings("unchecked")
> >>>> + final Comparable<TypeConverter<?>>
> >> comparableConflictingConverter =
> >>>> + (Comparable<TypeConverter<?>>)
> >> conflictingConverter;
> >>>> + overridable =
> >> comparableConflictingConverter.compareTo(converter) > 0;
> >>>> + } else {
> >>>> + overridable = false;
> >>>> + }
> >>>> + if (overridable) {
> >>>> + LOGGER.debug(
> >>>> + "Replacing TypeConverter [{}] for type [{}]
> >> with [{}] after comparison.",
> >>>> + conflictingConverter, conversionType,
> >> converter);
> >>>> + registry.put(conversionType, converter);
> >>>> + return converter;
> >>>> + } else {
> >>>> + LOGGER.warn(
> >>>> + "Ignoring TypeConverter [{}] for type [{}]
> >> that conflicts with [{}], since they are not comparable.",
> >>>> + converter, conversionType,
> >> conflictingConverter);
> >>>> + return conflictingConverter;
> >>>> }
> >>>> + } else {
> >>>> + registry.put(conversionType, converter);
> >>>> + return converter;
> >>>> }
> >>>> }
> >>>
> >>>
> >>>
> >>
> >>
> >>
>
>
>
Re: [logging-log4j2] branch release-2.x updated: LOG4J2-3004 Add
plugin support to JsonTemplateLayout. (#476)
Posted by Ralph Goers <ra...@dslextreme.com>.
OK, release-2.x now seems to be better but I don’t understand what is going on with Master. I am sure I built master several times over the weekend. But now it is getting errors all over the place in Json Template Layout tests. I did two commits that shouldn’t have impacted anything in that.
My only concern when code like the registry changes is that all the existing unit tests continue to pass without having to change them (unless they are clearly buggy). We have enough tests that I am pretty confident that if they all pass nothing that is going to affect anything has changed.
Ralph
> On Apr 5, 2021, at 1:19 PM, Volkan Yazıcı <vo...@gmail.com> wrote:
>
> Hello Ralph,
>
> *TypeConverterRegistry changes*
>
> Sorry for breaking certain tests. I had used the following to test my
> changes: `./mvnw clean install -pl
> :log4j-core,:log4j-layout-template-json`. I have tested my changes after
> purging Log4j `2.*-SNAPSHOT` artifacts from my `~/.m2` and there I indeed
> observe the same errors reported in Jenkins:
>
> org.apache.logging.log4j.core.config.LoggersPluginTest.testEmptyAttribute
> org.apache.logging.log4j.core.config.plugins.validation.validators.ValidatingPluginWithFailoverTest.testDoesNotLog_NoParameterThatMatchesElement_message
>
> (Apparently I have needed an extra bootstrap for annotations to kick in.)
>
> Though something interesting is going on here. If I revert my changes to
> TypeConverterRegistry, *aforementioned tests still do fail*! It is actually
> TypeConverterRegistryTest changes that is breaking those changes. I have
> experimented with the following combinations:
>
> - reverted TypeConverterRegistry ⇨ LoggersPluginTest fails
> - reverted TypeConverterRegistryTest ⇨ LoggersPluginTest succeeds
> - `@Ignore`d added tests to TypeConverterRegistryTest ⇨ LoggersPluginTest
> fails
>
> I have reverted the changes to TypeConverterRegistryTest, though the
> TypeConverterRegistry changes are still in. I hope this would suffice to
> make Jenkins and GitHub Actions happy.
>
> *RecyclerFactory customization support*
>
> In JsonTemplateLayout.Builder, I have a @PluginBuilderAttribute of type
> RecyclerFactory. The input string is converted to a RecyclerFactory using
> RecyclerFactoryConverter. To allow users to introduce their own
> RecyclerFactory implementation in a backward-compatible way, I thought of
> supporting multiple TypeConverter's in TypeConverterRegistry. The idea is
> to enhance TypeConverterRegistry such that in the presence of multiple
> TypeConverter's matching on the same footprint, TypeConverterRegistry will
> use the Comparable interface to determine the effective one.
>
> I think it is a nice feature to support multiple TypeConverter's, as my use
> case demonstrates. Do you agree? What else would you recommend me to
> implement this customization support needed for
> JsonTemplateLayout.Builder#recyclerFactory?
>
> Kind regards.
>
> On Sun, Apr 4, 2021 at 8:43 AM Ralph Goers <ra...@dslextreme.com>
> wrote:
>
>> I am now looking at the Jenkins and GitHub Actions builds and it is clear
>> that the change I highlighted below has caused at 3 tests to fail. Please
>> revert the change to the TypeConverterRegistry. In the future, whenever a
>> change is made to the API or core components please make sure you run a
>> full build before you commit. This should be standard practice for everyone.
>>
>> Ralph
>>
>>> On Apr 3, 2021, at 1:27 PM, Ralph Goers <ra...@dslextreme.com>
>> wrote:
>>>
>>> Volkan,
>>>
>>> I haven’t looked at the details of what you changed in
>> JsonTemplateLayout yet but the below concerns me a bit….
>>>
>>> Ralph
>>>
>>>
>>>> On Apr 3, 2021, at 3:48 AM, vy@apache.org wrote:
>>>>
>>>> This is an automated email from the ASF dual-hosted git repository.
>>>>
>>>
>>> Why were changes to the registry required for this? Aren’t you just
>> creating new Plugins?
>>>
>>>> diff --git
>> a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
>> b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
>>>> index 5088f15..3964370 100644
>>>> ---
>> a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
>>>> +++
>> b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
>>>> @@ -86,8 +86,9 @@ public class TypeConverterRegistry {
>>>> if (clazz.isEnum()) {
>>>> @SuppressWarnings({"unchecked","rawtypes"})
>>>> final EnumConverter<? extends Enum> converter = new
>> EnumConverter(clazz.asSubclass(Enum.class));
>>>
>>> I don’t understand how replacing calling registerConverter with
>> putIfAbsent is equivalent. The registerConverterMethod seems to allow
>> plugins to be overridden.
>>>
>>>> - registry.putIfAbsent(type, converter);
>>>> - return converter;
>>>> + synchronized (INSTANCE_LOCK) {
>>>> + return registerConverter(type, converter);
>>>> + }
>>>> }
>>>> }
>>>> // look for compatible converters
>>>> @@ -96,8 +97,9 @@ public class TypeConverterRegistry {
>>>> if (TypeUtil.isAssignable(type, key)) {
>>>> LOGGER.debug("Found compatible TypeConverter<{}> for
>> type [{}].", key, type);
>>>> final TypeConverter<?> value = entry.getValue();
>>>> - registry.putIfAbsent(type, value);
>>>> - return value;
>>>> + synchronized (INSTANCE_LOCK) {
>>>> + return registerConverter(type, value);
>>>> + }
>>>> }
>>>> }
>>>> throw new UnknownFormatConversionException(type.toString());
>>>> @@ -119,11 +121,52 @@ public class TypeConverterRegistry {
>>>> final Class<? extends TypeConverter> pluginClass =
>> clazz.asSubclass(TypeConverter.class);
>>>> final Type conversionType =
>> getTypeConverterSupportedType(pluginClass);
>>>> final TypeConverter<?> converter =
>> ReflectionUtil.instantiate(pluginClass);
>>>> - if (registry.putIfAbsent(conversionType, converter) !=
>> null) {
>>>> - LOGGER.warn("Found a TypeConverter [{}] for type
>> [{}] that already exists.", converter,
>>>> - conversionType);
>>>> - }
>>>> + registerConverter(conversionType, converter);
>>>> + }
>>>> + }
>>>> + }
>>>> +
>>>> + /**
>>>> + * Attempts to register the given converter and returns the
>> effective
>>>> + * converter associated with the given type.
>>>> + * <p>
>>>> + * Registration will fail if there already exists a converter for
>> the given
>>>> + * type and neither the existing, nor the provided converter
>> extends from {@link Comparable}.
>>>> + */
>>>> + private TypeConverter<?> registerConverter(
>>>> + final Type conversionType,
>>>> + final TypeConverter<?> converter) {
>>>> + final TypeConverter<?> conflictingConverter =
>> registry.get(conversionType);
>>>> + if (conflictingConverter != null) {
>>>> + final boolean overridable;
>>>> + if (converter instanceof Comparable) {
>>>> + @SuppressWarnings("unchecked")
>>>> + final Comparable<TypeConverter<?>> comparableConverter
>> =
>>>> + (Comparable<TypeConverter<?>>) converter;
>>>> + overridable =
>> comparableConverter.compareTo(conflictingConverter) < 0;
>>>> + } else if (conflictingConverter instanceof Comparable) {
>>>> + @SuppressWarnings("unchecked")
>>>> + final Comparable<TypeConverter<?>>
>> comparableConflictingConverter =
>>>> + (Comparable<TypeConverter<?>>)
>> conflictingConverter;
>>>> + overridable =
>> comparableConflictingConverter.compareTo(converter) > 0;
>>>> + } else {
>>>> + overridable = false;
>>>> + }
>>>> + if (overridable) {
>>>> + LOGGER.debug(
>>>> + "Replacing TypeConverter [{}] for type [{}]
>> with [{}] after comparison.",
>>>> + conflictingConverter, conversionType,
>> converter);
>>>> + registry.put(conversionType, converter);
>>>> + return converter;
>>>> + } else {
>>>> + LOGGER.warn(
>>>> + "Ignoring TypeConverter [{}] for type [{}]
>> that conflicts with [{}], since they are not comparable.",
>>>> + converter, conversionType,
>> conflictingConverter);
>>>> + return conflictingConverter;
>>>> }
>>>> + } else {
>>>> + registry.put(conversionType, converter);
>>>> + return converter;
>>>> }
>>>> }
>>>
>>>
>>>
>>
>>
>>
Re: [logging-log4j2] branch release-2.x updated: LOG4J2-3004 Add
plugin support to JsonTemplateLayout. (#476)
Posted by Matt Sicker <bo...@gmail.com>.
TypeConverterRegistry updates should be fine, just note I had to
structure some of the code as it is as it can't rely on much else
being set up quite yet. Even with the DI-rewrite, I think this area
would still have limited ability to inject any configuration state as
it's used for parsing and injecting the configuration in the first
place. They're in their own plugin category, so it's independent of
configuration elements anyways.
On Mon, 5 Apr 2021 at 15:19, Volkan Yazıcı <vo...@gmail.com> wrote:
>
> Hello Ralph,
>
> *TypeConverterRegistry changes*
>
> Sorry for breaking certain tests. I had used the following to test my
> changes: `./mvnw clean install -pl
> :log4j-core,:log4j-layout-template-json`. I have tested my changes after
> purging Log4j `2.*-SNAPSHOT` artifacts from my `~/.m2` and there I indeed
> observe the same errors reported in Jenkins:
>
> org.apache.logging.log4j.core.config.LoggersPluginTest.testEmptyAttribute
> org.apache.logging.log4j.core.config.plugins.validation.validators.ValidatingPluginWithFailoverTest.testDoesNotLog_NoParameterThatMatchesElement_message
>
> (Apparently I have needed an extra bootstrap for annotations to kick in.)
>
> Though something interesting is going on here. If I revert my changes to
> TypeConverterRegistry, *aforementioned tests still do fail*! It is actually
> TypeConverterRegistryTest changes that is breaking those changes. I have
> experimented with the following combinations:
>
> - reverted TypeConverterRegistry ⇨ LoggersPluginTest fails
> - reverted TypeConverterRegistryTest ⇨ LoggersPluginTest succeeds
> - `@Ignore`d added tests to TypeConverterRegistryTest ⇨ LoggersPluginTest
> fails
>
> I have reverted the changes to TypeConverterRegistryTest, though the
> TypeConverterRegistry changes are still in. I hope this would suffice to
> make Jenkins and GitHub Actions happy.
>
> *RecyclerFactory customization support*
>
> In JsonTemplateLayout.Builder, I have a @PluginBuilderAttribute of type
> RecyclerFactory. The input string is converted to a RecyclerFactory using
> RecyclerFactoryConverter. To allow users to introduce their own
> RecyclerFactory implementation in a backward-compatible way, I thought of
> supporting multiple TypeConverter's in TypeConverterRegistry. The idea is
> to enhance TypeConverterRegistry such that in the presence of multiple
> TypeConverter's matching on the same footprint, TypeConverterRegistry will
> use the Comparable interface to determine the effective one.
>
> I think it is a nice feature to support multiple TypeConverter's, as my use
> case demonstrates. Do you agree? What else would you recommend me to
> implement this customization support needed for
> JsonTemplateLayout.Builder#recyclerFactory?
>
> Kind regards.
>
> On Sun, Apr 4, 2021 at 8:43 AM Ralph Goers <ra...@dslextreme.com>
> wrote:
>
> > I am now looking at the Jenkins and GitHub Actions builds and it is clear
> > that the change I highlighted below has caused at 3 tests to fail. Please
> > revert the change to the TypeConverterRegistry. In the future, whenever a
> > change is made to the API or core components please make sure you run a
> > full build before you commit. This should be standard practice for everyone.
> >
> > Ralph
> >
> > > On Apr 3, 2021, at 1:27 PM, Ralph Goers <ra...@dslextreme.com>
> > wrote:
> > >
> > > Volkan,
> > >
> > > I haven’t looked at the details of what you changed in
> > JsonTemplateLayout yet but the below concerns me a bit….
> > >
> > > Ralph
> > >
> > >
> > >> On Apr 3, 2021, at 3:48 AM, vy@apache.org wrote:
> > >>
> > >> This is an automated email from the ASF dual-hosted git repository.
> > >>
> > >
> > > Why were changes to the registry required for this? Aren’t you just
> > creating new Plugins?
> > >
> > >> diff --git
> > a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> > b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> > >> index 5088f15..3964370 100644
> > >> ---
> > a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> > >> +++
> > b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> > >> @@ -86,8 +86,9 @@ public class TypeConverterRegistry {
> > >> if (clazz.isEnum()) {
> > >> @SuppressWarnings({"unchecked","rawtypes"})
> > >> final EnumConverter<? extends Enum> converter = new
> > EnumConverter(clazz.asSubclass(Enum.class));
> > >
> > > I don’t understand how replacing calling registerConverter with
> > putIfAbsent is equivalent. The registerConverterMethod seems to allow
> > plugins to be overridden.
> > >
> > >> - registry.putIfAbsent(type, converter);
> > >> - return converter;
> > >> + synchronized (INSTANCE_LOCK) {
> > >> + return registerConverter(type, converter);
> > >> + }
> > >> }
> > >> }
> > >> // look for compatible converters
> > >> @@ -96,8 +97,9 @@ public class TypeConverterRegistry {
> > >> if (TypeUtil.isAssignable(type, key)) {
> > >> LOGGER.debug("Found compatible TypeConverter<{}> for
> > type [{}].", key, type);
> > >> final TypeConverter<?> value = entry.getValue();
> > >> - registry.putIfAbsent(type, value);
> > >> - return value;
> > >> + synchronized (INSTANCE_LOCK) {
> > >> + return registerConverter(type, value);
> > >> + }
> > >> }
> > >> }
> > >> throw new UnknownFormatConversionException(type.toString());
> > >> @@ -119,11 +121,52 @@ public class TypeConverterRegistry {
> > >> final Class<? extends TypeConverter> pluginClass =
> > clazz.asSubclass(TypeConverter.class);
> > >> final Type conversionType =
> > getTypeConverterSupportedType(pluginClass);
> > >> final TypeConverter<?> converter =
> > ReflectionUtil.instantiate(pluginClass);
> > >> - if (registry.putIfAbsent(conversionType, converter) !=
> > null) {
> > >> - LOGGER.warn("Found a TypeConverter [{}] for type
> > [{}] that already exists.", converter,
> > >> - conversionType);
> > >> - }
> > >> + registerConverter(conversionType, converter);
> > >> + }
> > >> + }
> > >> + }
> > >> +
> > >> + /**
> > >> + * Attempts to register the given converter and returns the
> > effective
> > >> + * converter associated with the given type.
> > >> + * <p>
> > >> + * Registration will fail if there already exists a converter for
> > the given
> > >> + * type and neither the existing, nor the provided converter
> > extends from {@link Comparable}.
> > >> + */
> > >> + private TypeConverter<?> registerConverter(
> > >> + final Type conversionType,
> > >> + final TypeConverter<?> converter) {
> > >> + final TypeConverter<?> conflictingConverter =
> > registry.get(conversionType);
> > >> + if (conflictingConverter != null) {
> > >> + final boolean overridable;
> > >> + if (converter instanceof Comparable) {
> > >> + @SuppressWarnings("unchecked")
> > >> + final Comparable<TypeConverter<?>> comparableConverter
> > =
> > >> + (Comparable<TypeConverter<?>>) converter;
> > >> + overridable =
> > comparableConverter.compareTo(conflictingConverter) < 0;
> > >> + } else if (conflictingConverter instanceof Comparable) {
> > >> + @SuppressWarnings("unchecked")
> > >> + final Comparable<TypeConverter<?>>
> > comparableConflictingConverter =
> > >> + (Comparable<TypeConverter<?>>)
> > conflictingConverter;
> > >> + overridable =
> > comparableConflictingConverter.compareTo(converter) > 0;
> > >> + } else {
> > >> + overridable = false;
> > >> + }
> > >> + if (overridable) {
> > >> + LOGGER.debug(
> > >> + "Replacing TypeConverter [{}] for type [{}]
> > with [{}] after comparison.",
> > >> + conflictingConverter, conversionType,
> > converter);
> > >> + registry.put(conversionType, converter);
> > >> + return converter;
> > >> + } else {
> > >> + LOGGER.warn(
> > >> + "Ignoring TypeConverter [{}] for type [{}]
> > that conflicts with [{}], since they are not comparable.",
> > >> + converter, conversionType,
> > conflictingConverter);
> > >> + return conflictingConverter;
> > >> }
> > >> + } else {
> > >> + registry.put(conversionType, converter);
> > >> + return converter;
> > >> }
> > >> }
> > >
> > >
> > >
> >
> >
> >
Re: [logging-log4j2] branch release-2.x updated: LOG4J2-3004 Add
plugin support to JsonTemplateLayout. (#476)
Posted by Volkan Yazıcı <vo...@gmail.com>.
Hello Ralph,
*TypeConverterRegistry changes*
Sorry for breaking certain tests. I had used the following to test my
changes: `./mvnw clean install -pl
:log4j-core,:log4j-layout-template-json`. I have tested my changes after
purging Log4j `2.*-SNAPSHOT` artifacts from my `~/.m2` and there I indeed
observe the same errors reported in Jenkins:
org.apache.logging.log4j.core.config.LoggersPluginTest.testEmptyAttribute
org.apache.logging.log4j.core.config.plugins.validation.validators.ValidatingPluginWithFailoverTest.testDoesNotLog_NoParameterThatMatchesElement_message
(Apparently I have needed an extra bootstrap for annotations to kick in.)
Though something interesting is going on here. If I revert my changes to
TypeConverterRegistry, *aforementioned tests still do fail*! It is actually
TypeConverterRegistryTest changes that is breaking those changes. I have
experimented with the following combinations:
- reverted TypeConverterRegistry ⇨ LoggersPluginTest fails
- reverted TypeConverterRegistryTest ⇨ LoggersPluginTest succeeds
- `@Ignore`d added tests to TypeConverterRegistryTest ⇨ LoggersPluginTest
fails
I have reverted the changes to TypeConverterRegistryTest, though the
TypeConverterRegistry changes are still in. I hope this would suffice to
make Jenkins and GitHub Actions happy.
*RecyclerFactory customization support*
In JsonTemplateLayout.Builder, I have a @PluginBuilderAttribute of type
RecyclerFactory. The input string is converted to a RecyclerFactory using
RecyclerFactoryConverter. To allow users to introduce their own
RecyclerFactory implementation in a backward-compatible way, I thought of
supporting multiple TypeConverter's in TypeConverterRegistry. The idea is
to enhance TypeConverterRegistry such that in the presence of multiple
TypeConverter's matching on the same footprint, TypeConverterRegistry will
use the Comparable interface to determine the effective one.
I think it is a nice feature to support multiple TypeConverter's, as my use
case demonstrates. Do you agree? What else would you recommend me to
implement this customization support needed for
JsonTemplateLayout.Builder#recyclerFactory?
Kind regards.
On Sun, Apr 4, 2021 at 8:43 AM Ralph Goers <ra...@dslextreme.com>
wrote:
> I am now looking at the Jenkins and GitHub Actions builds and it is clear
> that the change I highlighted below has caused at 3 tests to fail. Please
> revert the change to the TypeConverterRegistry. In the future, whenever a
> change is made to the API or core components please make sure you run a
> full build before you commit. This should be standard practice for everyone.
>
> Ralph
>
> > On Apr 3, 2021, at 1:27 PM, Ralph Goers <ra...@dslextreme.com>
> wrote:
> >
> > Volkan,
> >
> > I haven’t looked at the details of what you changed in
> JsonTemplateLayout yet but the below concerns me a bit….
> >
> > Ralph
> >
> >
> >> On Apr 3, 2021, at 3:48 AM, vy@apache.org wrote:
> >>
> >> This is an automated email from the ASF dual-hosted git repository.
> >>
> >
> > Why were changes to the registry required for this? Aren’t you just
> creating new Plugins?
> >
> >> diff --git
> a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> >> index 5088f15..3964370 100644
> >> ---
> a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> >> +++
> b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
> >> @@ -86,8 +86,9 @@ public class TypeConverterRegistry {
> >> if (clazz.isEnum()) {
> >> @SuppressWarnings({"unchecked","rawtypes"})
> >> final EnumConverter<? extends Enum> converter = new
> EnumConverter(clazz.asSubclass(Enum.class));
> >
> > I don’t understand how replacing calling registerConverter with
> putIfAbsent is equivalent. The registerConverterMethod seems to allow
> plugins to be overridden.
> >
> >> - registry.putIfAbsent(type, converter);
> >> - return converter;
> >> + synchronized (INSTANCE_LOCK) {
> >> + return registerConverter(type, converter);
> >> + }
> >> }
> >> }
> >> // look for compatible converters
> >> @@ -96,8 +97,9 @@ public class TypeConverterRegistry {
> >> if (TypeUtil.isAssignable(type, key)) {
> >> LOGGER.debug("Found compatible TypeConverter<{}> for
> type [{}].", key, type);
> >> final TypeConverter<?> value = entry.getValue();
> >> - registry.putIfAbsent(type, value);
> >> - return value;
> >> + synchronized (INSTANCE_LOCK) {
> >> + return registerConverter(type, value);
> >> + }
> >> }
> >> }
> >> throw new UnknownFormatConversionException(type.toString());
> >> @@ -119,11 +121,52 @@ public class TypeConverterRegistry {
> >> final Class<? extends TypeConverter> pluginClass =
> clazz.asSubclass(TypeConverter.class);
> >> final Type conversionType =
> getTypeConverterSupportedType(pluginClass);
> >> final TypeConverter<?> converter =
> ReflectionUtil.instantiate(pluginClass);
> >> - if (registry.putIfAbsent(conversionType, converter) !=
> null) {
> >> - LOGGER.warn("Found a TypeConverter [{}] for type
> [{}] that already exists.", converter,
> >> - conversionType);
> >> - }
> >> + registerConverter(conversionType, converter);
> >> + }
> >> + }
> >> + }
> >> +
> >> + /**
> >> + * Attempts to register the given converter and returns the
> effective
> >> + * converter associated with the given type.
> >> + * <p>
> >> + * Registration will fail if there already exists a converter for
> the given
> >> + * type and neither the existing, nor the provided converter
> extends from {@link Comparable}.
> >> + */
> >> + private TypeConverter<?> registerConverter(
> >> + final Type conversionType,
> >> + final TypeConverter<?> converter) {
> >> + final TypeConverter<?> conflictingConverter =
> registry.get(conversionType);
> >> + if (conflictingConverter != null) {
> >> + final boolean overridable;
> >> + if (converter instanceof Comparable) {
> >> + @SuppressWarnings("unchecked")
> >> + final Comparable<TypeConverter<?>> comparableConverter
> =
> >> + (Comparable<TypeConverter<?>>) converter;
> >> + overridable =
> comparableConverter.compareTo(conflictingConverter) < 0;
> >> + } else if (conflictingConverter instanceof Comparable) {
> >> + @SuppressWarnings("unchecked")
> >> + final Comparable<TypeConverter<?>>
> comparableConflictingConverter =
> >> + (Comparable<TypeConverter<?>>)
> conflictingConverter;
> >> + overridable =
> comparableConflictingConverter.compareTo(converter) > 0;
> >> + } else {
> >> + overridable = false;
> >> + }
> >> + if (overridable) {
> >> + LOGGER.debug(
> >> + "Replacing TypeConverter [{}] for type [{}]
> with [{}] after comparison.",
> >> + conflictingConverter, conversionType,
> converter);
> >> + registry.put(conversionType, converter);
> >> + return converter;
> >> + } else {
> >> + LOGGER.warn(
> >> + "Ignoring TypeConverter [{}] for type [{}]
> that conflicts with [{}], since they are not comparable.",
> >> + converter, conversionType,
> conflictingConverter);
> >> + return conflictingConverter;
> >> }
> >> + } else {
> >> + registry.put(conversionType, converter);
> >> + return converter;
> >> }
> >> }
> >
> >
> >
>
>
>
Re: [logging-log4j2] branch release-2.x updated: LOG4J2-3004 Add
plugin support to JsonTemplateLayout. (#476)
Posted by Ralph Goers <ra...@dslextreme.com>.
I am now looking at the Jenkins and GitHub Actions builds and it is clear that the change I highlighted below has caused at 3 tests to fail. Please revert the change to the TypeConverterRegistry. In the future, whenever a change is made to the API or core components please make sure you run a full build before you commit. This should be standard practice for everyone.
Ralph
> On Apr 3, 2021, at 1:27 PM, Ralph Goers <ra...@dslextreme.com> wrote:
>
> Volkan,
>
> I haven’t looked at the details of what you changed in JsonTemplateLayout yet but the below concerns me a bit….
>
> Ralph
>
>
>> On Apr 3, 2021, at 3:48 AM, vy@apache.org wrote:
>>
>> This is an automated email from the ASF dual-hosted git repository.
>>
>
> Why were changes to the registry required for this? Aren’t you just creating new Plugins?
>
>> diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
>> index 5088f15..3964370 100644
>> --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
>> +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/convert/TypeConverterRegistry.java
>> @@ -86,8 +86,9 @@ public class TypeConverterRegistry {
>> if (clazz.isEnum()) {
>> @SuppressWarnings({"unchecked","rawtypes"})
>> final EnumConverter<? extends Enum> converter = new EnumConverter(clazz.asSubclass(Enum.class));
>
> I don’t understand how replacing calling registerConverter with putIfAbsent is equivalent. The registerConverterMethod seems to allow plugins to be overridden.
>
>> - registry.putIfAbsent(type, converter);
>> - return converter;
>> + synchronized (INSTANCE_LOCK) {
>> + return registerConverter(type, converter);
>> + }
>> }
>> }
>> // look for compatible converters
>> @@ -96,8 +97,9 @@ public class TypeConverterRegistry {
>> if (TypeUtil.isAssignable(type, key)) {
>> LOGGER.debug("Found compatible TypeConverter<{}> for type [{}].", key, type);
>> final TypeConverter<?> value = entry.getValue();
>> - registry.putIfAbsent(type, value);
>> - return value;
>> + synchronized (INSTANCE_LOCK) {
>> + return registerConverter(type, value);
>> + }
>> }
>> }
>> throw new UnknownFormatConversionException(type.toString());
>> @@ -119,11 +121,52 @@ public class TypeConverterRegistry {
>> final Class<? extends TypeConverter> pluginClass = clazz.asSubclass(TypeConverter.class);
>> final Type conversionType = getTypeConverterSupportedType(pluginClass);
>> final TypeConverter<?> converter = ReflectionUtil.instantiate(pluginClass);
>> - if (registry.putIfAbsent(conversionType, converter) != null) {
>> - LOGGER.warn("Found a TypeConverter [{}] for type [{}] that already exists.", converter,
>> - conversionType);
>> - }
>> + registerConverter(conversionType, converter);
>> + }
>> + }
>> + }
>> +
>> + /**
>> + * Attempts to register the given converter and returns the effective
>> + * converter associated with the given type.
>> + * <p>
>> + * Registration will fail if there already exists a converter for the given
>> + * type and neither the existing, nor the provided converter extends from {@link Comparable}.
>> + */
>> + private TypeConverter<?> registerConverter(
>> + final Type conversionType,
>> + final TypeConverter<?> converter) {
>> + final TypeConverter<?> conflictingConverter = registry.get(conversionType);
>> + if (conflictingConverter != null) {
>> + final boolean overridable;
>> + if (converter instanceof Comparable) {
>> + @SuppressWarnings("unchecked")
>> + final Comparable<TypeConverter<?>> comparableConverter =
>> + (Comparable<TypeConverter<?>>) converter;
>> + overridable = comparableConverter.compareTo(conflictingConverter) < 0;
>> + } else if (conflictingConverter instanceof Comparable) {
>> + @SuppressWarnings("unchecked")
>> + final Comparable<TypeConverter<?>> comparableConflictingConverter =
>> + (Comparable<TypeConverter<?>>) conflictingConverter;
>> + overridable = comparableConflictingConverter.compareTo(converter) > 0;
>> + } else {
>> + overridable = false;
>> + }
>> + if (overridable) {
>> + LOGGER.debug(
>> + "Replacing TypeConverter [{}] for type [{}] with [{}] after comparison.",
>> + conflictingConverter, conversionType, converter);
>> + registry.put(conversionType, converter);
>> + return converter;
>> + } else {
>> + LOGGER.warn(
>> + "Ignoring TypeConverter [{}] for type [{}] that conflicts with [{}], since they are not comparable.",
>> + converter, conversionType, conflictingConverter);
>> + return conflictingConverter;
>> }
>> + } else {
>> + registry.put(conversionType, converter);
>> + return converter;
>> }
>> }
>
>
>