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

[Mandy Chung]

> 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.  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)?   Can custom LogManager call updateConfiguration in their constructor instead of overriding readConfiguration?  If the existing readConfiguration methods should be deprecated, i think keeping the same method name may be a better choice.

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.

1761      * @param remapper 

“re” probably not needed here.   I think “mapper” should work.

Use @throws instead of @exception

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?

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?  

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). 

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

Mandy