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

[Mandy Chung]

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

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/

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},"

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

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.

Also @param mapper - need to say "mapper may be null"

Same comment applies to @param in the other updateConfiguration method.


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.

1842      * listener}

Should it be "listener(s)"

2018             // Builds a TreeSet of all (old and new) property names.
2030                         .forEachOrdered(k -> ConfigProperty

Is the ordering matter here?

2071                     if (l == null) continue;
2072                     if (!visited.test(l)) {

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

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.

Mandy