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

[Daniel Fuchs]

Hi Mandy,

Here is a new webrev [1] - thanks a lot for all the time you
spent working with me on the API documentation!

As you suggested:

The specification of how readConfiguration uses system
properties to find the configuration has been
moved from the class-level documentation to the method
documentation.
The behaviour of readConfiguration has been clarified,
in particular with respect to what it does (or does not
do) with existing loggers.
The documentation of reset() has also been clarified
with respect to its effect on existing loggers.

[These are only clarification of the spec, there's no behavior
  change]

The specification of the new updateConfiguration has been
improved and made more consistent.

I have also implemented the few internal variable/class/method
renaming you suggested.

[1] webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8033661/webrev.02

best regards,

-- daniel

On 17/09/15 06:52, Mandy Chung wrote:
>
>> On Sep 14, 2015, at 9:25 AM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
>> So with that in mind, I have slightly altered the @apiNotes:
>>
>> in readConfiguration():
>>
>>      * @apiNote {@code readConfiguration} is principally useful for
>>      *    establishing the LogManager primordial configuration.
>>      *    <p>
>>      *    Calling this method directly from the application code after the
>>      *    LogManager has been initialized is discouraged.
>>
>
> What about:
>
> This method is intended for LogManager primordial configuration.  Use
> the {@link #updateConfiguration} method to reload configuration after
> the LogManager has been initialized.
>
> I think it’s worth cleaning up the specification about logging configuration - moving the rules about locating the configuration properties from the class javadoc to the specification of the readConfiguration method
>
> for example, move these 3 paragraphs
>    If the "java.util.logging.config.class" property is set, …..
>    If "java.util.logging.config.class" property is not set, ..
>    If neither of these properties is defined
>
>   to replace "The same rules are used for locating the configuration properties as are used at startup…..”.
>
> Note that readConfiguration() reads from system property.
>
>>
>> The rationale is that an application which needs to establish
>> a custom configuration should probably use the
>> {@code "java.util.logging.config.class"} property, and
>> call LogManager.readConfiguration(InputStream) from there,
>> as hinted in the class-level documentation.
>>
>> in readConfiguration(InputStream):
>
> This will not reinitialize the logging configuration specified in the "java.util.logging.config.class” system property.  So I think moving the rules to locate logging configuration to readConfiguration() method will help.
>
> What does this line do:
>    String names[] = parseClassNames("config”);
>
>
>>
>>      * @apiNote {@code readConfiguration} is principally useful for applications
>>      *    which use the {@code "java.util.logging.config.class"} property
>>      *    to establish their own custom configuration.
>>      *    <p>
>>      *    Calling this method directly from the application code after the
>>      *    LogManager has been initialized is discouraged.
>
> Since it’s not deprecated, instead of a note to discourage using it, it seems to be more useful to describe the behavior of this method and issues with the handler etc after reset.
>
> Then recommend to use the {@link #updateConfiguration} method to reload configuration after
> the LogManager has been initialized.
>
>>
>>>
>>> 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.
>>
>> I updated the doc for updateConfiguration(mapper):
>>
>>   :
>>
>
> I think this method should only read the logging config file if the "java.util.logging.config.class” property is not set.  If set, it should be instantiated successfully and that will never read the logging config file.
>
> The spec should say:
>
> Update the logging configuration.  It will read the logging properties file that was read at startup time.
>
> If the "java.util.logging.config.file" system property is set, the logging configuration is read from the path specified in the property value.  If the "java.util.logging.config.file" system property is not set, then the default configuration is used.
>
> Question:
> If “java.util.logging.config.class” is set, should it throw an exception instead of silently continue or read from the default configuration.
>
> Should it re-read the system property at runtime or cached value?  What if  “java.util.logging.config.file” is not set at startup but later set at runtime after LogManager was initialized?
>
>>
>>
>>>
>>> 1761      * @param remapper
>>>
>>> “re” probably not needed here.   I think “mapper” should work.
>>
>> OK. Should I still use the term of 'remapping function' then?
>> Or does that become 'mapping function' too?
>>
>
> Can it simply refer it as “a function mapping from a pair of the current value and the new value of a key to the value to be applied"
>
> What about something like this:
>
> mapper - a functional interface that takes a configuration key and returns a function that takes the value in the current configuration and the value in the given configuration as parameters and produces the actual value to be applied. If the mapper is null, or the function the mapper returns is null, then the value in the given configuration will be the new value. A null value passed as parameter to the function indicates that no value was present in the corresponding configuration.
>
> Another way can define some variables to represent all these values and functions so that the specification can refer to these variables instead.  I can work with you on the specification.
>
>
>> http://cr.openjdk.java.net/~dfuchs/webrev_8033661/webrev.01/
>>
>
> Implementation side - looks okay.  Some comments:
>
> I found some VisitedLoggers methods unnecessary.  It may help if we can change
>    VisitedLoggers.test(Logger) - name can be obtained from Logger.getName()
>    VisitedLoggers.
>
> Instead of calling VisitedLoggers.of(null), it seems more explicit to call VisitedLogger.NEVER (or FALSE)
>
> Is it useful to have ModType.EQUAL or SAME and non null value guaranteed.
>
> Perhaps time to change LoggerContext.getLoggerNames to return Set<String>.
>
> ConfigurationProperties - it’s an enum, should it be ConfigurationProperty instead?  ConfigProperty might work too.
>
> ConfigurationProperties.isChanging method - would “compare” and “equals” work here?  Using some commonly used method names will help readers to understand the code easier.
>
> final List<Logger> tmp = cxs.isEmpty() ….
> - would “loggers” work here - anything better than tmp?  It’s actually the list of candidate loggers to be updated with the new configuration properties.
>
> There are several words referenced “reestablished”, “reinitialized”, “update”.  It’d help if the same word is used consistently in the specification and comment.
>
> I haven’t reviewed the tests yet.
> Mandy
>
>