RFR: 8033661: readConfiguration does not cleanly reinitialize the logging system
Daniel Fuchs
daniel.fuchs at oracle.com
Wed Sep 23 14:50:40 UTC 2015
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
>
More information about the core-libs-dev
mailing list