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