> On Sep 9, 2015, at 9:55 AM, Daniel Fuchs <daniel.fu...@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
