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

[Daniel Fuchs]

Hi Mandy,

On 23/09/15 01:02, Mandy Chung wrote:
> On 09/22/2015 09:27 AM, Daniel Fuchs wrote:
>>
>> http://cr.openjdk.java.net/~dfuchs/webrev_8033661/webrev.02
>
> Looking quite good.  Thanks for making the change.
>
> Below are mostly javadoc comments and some minor implementation comments.
>
> 1474      * which should be in {@linkplain java.util.Properties
> properties file} format.
>
> Would it be better to state this above in @throws IOException:
>
> IOException if there are problems reading from the stream or
> the given stream is not in Properties format

Done.

> 1484      * existing loggers that have a level property specified in the
> new
> 1485      * configuration stream
>
> s/the new configuration stream/the given input stream/
>
> 1491      * methods instead.
>
> s/methods/method/

Done.

>
> updateConfiguration(Function) - slight reorg of the sentences:
>
>       * Updates the logging configuration.
>       * <p>
>       * If the "java.util.logging.config.file" system property is set,
>       * then the property value specifies the properties file to be read
>       * as the new configuration. Otherwise, the LogManager default
>       * configuration is used.
>       * The default configuration is typically loaded from the
>       * properties file "{@code conf/logging.properties}" in the Java
> installation
>       * directory.
>       * This method reads the new configuration and calls the {@link
>       * #updateConfiguration(java.io.InputStream,
> java.util.function.Function)
>       * updateConfiguration(ins, mapper)} method to
>       * update the configuration.
>       *
>       * @apiNote
>       * This method updates the logging configuration from reading
>       * a properties file and ignores the "java.util.logging.config.class"
>       * system property.  The "java.util.logging.config.class" property is
>       * only used by the readConfiguration method to load a custom
> configuration
>       * class as an initial configuration.
>
> 1774      *   For each configuration key <i>k</i>,
>     Could simply be "For each {@code k},"

Done.

> - do you want italic for all occurrance?  Maybe just the first occurrance
>    and @code for the subsequent ones.

Mixing italics and {@code } was a bit confusing to me. When I generated
the javadoc and looked at the specdiff, it looked as if {@code k} was a 
different thing than the <i>k</i> we had discussed earlier.
So I choose to use italics for formal concepts/hypothetical variables
and {@code } for actual code and @param names.

> 1780      *   A {@code null} value for <i>v</i>  indicates that there
> should be no
> 1781      *   value associated with <i>k</i> in the resulting
> configuration.
>
> This sentence can be combined with line 1772-1773 as follows:
>
> returns a function <i>f(o,n)</i> whose returnedvalue will be
> applied to the resulting configuration, or {@code null}
> to indicate that the property {@code k}  will not be added
> in the resulting configuration.

OK - but it would be better to break the sentence:

returns a function <i>f(o,n)</i> whose returned
value will be applied to the resulting configuration.
The function <i>f</i> may return {@code null} to indicate
that the property <i>k</i> should not be added to the
resulting configuration.


> Also @param mapper - need to say "mapper may be null"
>
> Same comment applies to @param in the other updateConfiguration method.

right.


> 1825      * then <i>v = f(o,n)</i> is the
> 1826      * resulting value (i.e. the value that will be associated with
> 1827      * <i>k</i> in the resulting configuration).
>
> Might be something like this:
>
> then <i>v = f(o,n)</i> is the returned  value.
> If v is not null, the property k with value v will be added in
> the resulting configuration.  Otherwise, it will not be added.

then <i>v = f(o,n)</i> is the resulting value.
If <i>v</i> is not {@code null}, then a property
<i>k</i> with value <i>v</i> will be added to the resulting 
configuration. Otherwise, it will be omitted.

>
> 1842      * listener}
>
> Should it be "listener(s)"

OK.

> 2018             // Builds a TreeSet of all (old and new) property names.
> 2030                         .forEachOrdered(k -> ConfigProperty
>
> Is the ordering matter here?

Not really. It does matter later - we want to handle
parent loggers before child loggers to optimize the
processing but we use a TreeMap for that.
TreeSet is probably a remnant from an earlier prototype.


> 2071                     if (l == null) continue;
> 2072                     if (!visited.test(l)) {
>
> Nit: can be combined in one line
>     if (l != null && !visited.test(l)) {

Done.

>
> ah - I saw that is used in line 2166
>
> New tests: I only skimmed through them. They are good and covers complex
> scenarios.  For new methods like this, I generally like to see simple
> and small unit tests to be added that are easy for readers to
> understand.  For example, some sample properties files and check the
> resulting configuration using LogManager.getProperty. You already have
> many test cases covered.  I didn't want to add more works but it'd be
> really nice to see simple small unit tests.  Maybe refactor
> UpdateConfigurationTest.

OK - I will add a simple test. I think I'll probably use the
mapper examples from the documentation as my test cases :-)

I'll send a new webrev when ready.

-- daniel

>
> Mandy
>