RFR: JDK-8046565: Platform Logging API and Service

Tue Oct 13 12:14:19 UTC 2015

Hi Mandy,

Thanks a lot for your feedback!

On 13/10/15 04:50, Mandy Chung wrote:
> Hi Daniel,
>
>> On Oct 9, 2015, at 12:53 PM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
>>
>> JEP:
>> http://openjdk.java.net/jeps/264
>> https://bugs.openjdk.java.net/browse/JDK-8046565
>>
>> specdiff:
>> http://cr.openjdk.java.net/~dfuchs/8046565/proto.01/specdiff-api/overview-summary.html
>>
>> webrev:
>> http://cr.openjdk.java.net/~dfuchs/8046565/proto.01/webrev.01/
>
>
> This is very good work.  I’m glad to see this work that enables the dependency on java.logging be eliminated.  The sun.util.logging.PlatformLogger was a stop-gap approach.
>
> There are many new tests that I will need time to review them all.

I tried to split the tests into tests that only test the public API,
and tests that are implementation dependent: those also access
the internals or test the internal artifacts that the implementation
is using.
The split may not be obvious at the first glance - but tests under
'internal' or 'jdk' subdirs are implementation specific.
The other tests should not use internal APIs.
I have tried to convey the intent of the test in the @summary tag,
and those tests that are implementation specific say so in their
summary.

>  I’m going to pass you my comments in several batches.  This is the first batch.
>
> LogManager.java
> 1938         if (caller.getClassLoader() == null) {
> 1939             return LogManager.getLogManager().demandSystemLogger(name,
> 1940                 Logger.SYSTEM_LOGGER_RB_NAME, caller);
>
> This only considers the classes loaded by the boot loader as system classes.  We have deprivileged several modules to be loaded by the ext class loader such as JAX-WS, JAXB, CORBA etc.  Loggers created by modules loaded by boot and ext class loader should be system.

Right. I agree. But I also think this is orthogonal to this
JEP implementation. Would you agree to handle that in a separate
JBS issue (probably more 'Bug' than 'RFE')?

> In the absence of java.logging, the default provider implementation will be used to print the log messages to System.err.  This is useful since most JDK log messages are for debugging.  I expect that a component may want to turn on debug messages without requiring the presence of java.logging.
>
> Is there any mechanism to change the default level of the default simple console loggers?  It may be useful; otherwise it would need  to run on an image with java.logging module.  Maybe something to add in the future.

There isn't at this time. We could add for instance a global system
property that would allow to define the 'default level' for all
SimpleConsoleLogger. Something like '-Djdk.loggers.level.default=DEBUG'
which would have an effect similar to setting the root logger level
.level=FINE in the default configuration.
The question then is whether that property would only be used
when java.logging is not present, or whether the LogManager should
be modified to take this into account too - and whether it would have
precedence over what is in the configuration (default or not)...

Possibly something to handle in a second time. Should I add a subtask
to JDK-8046565 - to log an RFE about that after the initial
implementation gets in?

> System.Logger
> 964      * Unless
> - incomplete sentence?

Thanks for catching that. I'd been considering adding some kind of
global blanket statement for the throwing of NullPointerException,
then decided against it but forgot I had started writing the sentence...
I will remove this line.

> 1697      * @implSpec
> 1698      * Instances returned by this method route messages to loggers
> 1699      * obtained by calling {@link LoggerFinder#getLogger(java.lang.String, java.lang.Class)
> 1700      * LoggerFinder.getLogger(name, ca
>
> Is this intended to be implementation specification but not API spec?

It's intended to be an API spec that the messages should be routed
to loggers obtained from LoggerFinder.getLogger(name, caller).

Whether the logger returned is directly the logger obtained, or
a wrapper, and at which point exactly the 'real' logger will be created
is implementation dependent. I think we need to have a bit of freedom
here to deal with bootstrap issues according to the needs of the
modules in the JDK.

> RuntimePermission(“LoggerFinder) is required in the provider constructor.
> - is it time to define a ServiceProviderPermission for provider constructor security permission check?  Rather than overloading RuntimePermission

If there was a ServiceProviderPermission I would use it, but I
don't think this JEP should introduce it. We could log another
RFE for that.

> public static final RuntimePermission LOGGERFINDER_PERMISSION =
>           new RuntimePermission("loggerFinder”);
> - is there any need to define this constant?  Suggest this be implementation-details.

This was mostly temporary convenience for me - until I'm sure
I won't get further feedback that the name should be changed.
Avoids copy/paste typo mistakes and spending time debugging it.
If you think it would be better to not define such a constant,
then I agree to remove it in the final API.
Should I had a subtask to the JEP to get this removed, so that
it's not forgotten?

> private LoggerFinderLoader() {
>       throw new InternalError("LoggerFinderLoader cannot be instantiated");
> }
> - is throwing InternalError necessary?  No one else except its enclosing class can construct it.

It clearly indicates that it should not be instantiated.
I like it :-)

> JdkLoggingProvider::LOGGING_CONTROL_PERMISSION
> - I think this field is unused.

Thanks. Removed.

> sun/management/ManagementFactoryHelper.java
> - Is LoggingMXBeanSupport intended to get  java.management to remove its dependency on java.logging?
>
>    172     public interface LoggingMXBean
>    173         extends PlatformLoggingMXBean, java.util.logging.LoggingMXBean {
>    174     }
>
> This static dependency will still keep the dependency on java.logging.

Yes. I have been scratching my head trying to figure out how to
remove it.
If at some point we change java.management to use System.Logger instead
of java.util.logging.Logger, then we will still need
to specify that java.management requires java.logging - because
of this.
Maybe we can find a clever way to deprecate this and use
ServiceLoader to load the LogginMXBean implementation.
That would be preferable to the reflection tricks I had to add
here, but it might not be a trivial task.

However the code I added to guard the uses of PlatformLoggingImpl
and ManagemetFactoryHelper.LoggingMXBean should make it possible
to use java.management without java.logging - in the hypothesis
where java.management would be migrated to use System.Logger.

Though of course I haven't tested it. Anyway - that's
something for later.

best regards,

-- daniel

>
> Mandy
>