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ı <volkan.yaz...@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 <ralph.go...@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 <ralph.go...@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, v...@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;
>>>> }
>>>> }
>>>
>>>
>>>
>>
>>
>>
