RFR: 8033661: readConfiguration does not cleanly reinitialize the logging system

[Daniel Fuchs]

Hi Mandy,

Thanks a lot for the feedback!

On 13/09/15 00:44, Mandy Chung wrote:
>
>> On Sep 9, 2015, at 9:55 AM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
>>
>> Hi,
>>
>> Please find below a candidate fix for:
>>
>> 8033661: readConfiguration does not cleanly reinitialize the
>>          logging system
>> https://bugs.openjdk.java.net/browse/JDK-8033661
>>
>> http://cr.openjdk.java.net/~dfuchs/webrev_8033661/webrev.00/
>>
>> LogManager.readConfiguration() presents a number of flaws that may not
>> be fixable without endangering backward compatibility, or stability.
>> Some of them have been quite aptly summed up in JDK-8033661.
>
> Thanks for the detailed analysis.  I agree that adding a new method to
> reinitialize the logging configuration is the right thing to do.
>
> Have you considered keeping the same method name, readConfiguration with
> the new remapper parameter?  The downside is the difference that the
> reset is not invoked.

No - I didn't consider it. The algorithm implemented by the new
method is quite different from what was implemented in the
previous readConfiguration() methods - so the idea of overloading
readConfiguration() didn't come to my mind.

>  It might not matter because as you add in @apiNote that the existing
>  readConfiguration method may be overridden for custom LogManager but
>  discouraged to be used by applications.

> Related question: should the existing readConfiguration methods be
> deprecated and recommend to use the new method(s)?

What I see is that the old readConfiguration does the appropriate job
when called for the first time, before any logger has been created.
There is probably some application out there that override it to
install their own configuration.
For this reason I am not sure if it should be deprecated.

>  Can custom LogManager call updateConfiguration in their constructor
>  instead of overriding readConfiguration?

I believe that would be bad - it would go against what we've been
trying to do with https://bugs.openjdk.java.net/browse/JDK-8023168


> If the existing readConfiguration methods should be deprecated,
> i think keeping the same method name may be a better choice.

I'll leave the decision for that (keeping the same name) to you.
I'm not sure we can deprecate the old methods.
IMHO it is difficult to deprecate a method that is actually
called internally by LogManager, and stop calling it would be
risky WRT to compatibility.


>
> 1749      * Updates an existing configuration.
>
> We should specify what “existing configuration” is.
> If “java.util.logging.config.class” is set and it’s instantiated successfully,
> readConfiguration will not read from any configuration file.
> updateConfiguration only reads the config file regardless if
> “java.util.logging.config.class” is set.

Right. That's a good point.

> 1761      * @param remapper
>
> “re” probably not needed here.   I think “mapper” should work.

OK


> Use @throws instead of @exception

OK

>
> VisitedLogger class:
> 1714         public static VisitedLoggers of(Map<Logger, String> visited) {
> 1715             assert visited == null || visited instanceof IdentityHashMap;
> 1716             return visited == null ? NEVER : new VisitedLoggers(visited);
> 1717         }
>
> Instead of doing the assert, what about changing the parameter type to IdentityHashMap?

can do.

>
> Is VisitedLoggers class necessary?  Would it be more explicit to readers to use a Set (it doesn’t need to be a map) that each logger is visited once?

We don't have an IdentityHashSet - which is the reason while I'm using
an IdentityHashMap here.

> ConfigurationProperties class: This enum class is also a Predicate and
> it has several static methods for updateConfiguratoin to use e.g.
>
> final Stream<String> allKeys = updatePropertyNames.stream()
>          .filter(ConfigurationProperties::isOf)
>          .filter(k -> ConfigurationProperties.isChanging(k, previous, next));
>
> I have to read the comment and follow the implementation of these methods to figure
> out what it’s going on here.  It could be simplified and made the code easily
> to tell what are being filtered here.

> It seems to me that you want to define LevelProperty, HandlerProperty, UseParentProperty types,
> each of which will handle any change (as it’s currently done in the switch statement).

yes - which is why the enum is useful - as it allows to model all
these.

>
> ConfigurationProperties.ALL - why not use ConfigurationProperties.values()?

Well - I could reverse the question :-)

best regards,

-- daniel


>
> Mandy
>