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