RFR: JDK-8046565: Platform Logging API and Service

[Mandy Chung]

> On Oct 13, 2015, at 5:14 AM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
> 
> Hi Mandy,
> 
> 
> Thanks a lot for your feedback!
> 
> On 13/10/15 04:50, Mandy Chung wrote:
>> 
>> 
>> There are many new tests that I will need time to review them all.
> 
> :

My typo s/new tests/new classes/

> 
>> 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’)?

I recalled you specify this in the specification.  I prefer this to be handled at the moment.   I currently focus on reviewing the implementation.  I will re-comment on this point.

> 
>> 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?

That’d be good.

> 
>> 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).

You can take out @implSpec in that case.

> 
> 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.
> 

I don’t see any issue of defining the permission type.  I also have to understand other permission checks required for the provider implementation and may be an option to use a new permission specific to logger finder.   Stay tuned.

> 
>> 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.

My take is that custom provider implementation can construct this instance themselves.  You can define this constant as implementation.

> Should I had a subtask to the JEP to get this removed, so that
> it's not forgotten?
> 

Are you thinking this is an API change?  If you plan to address this in the first integration of this JEP, there is no need to file a subtask.  It’s up to you if you prefer filing a subtask tracking the review comments for your own use.

> 
>> 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 :-)

Maybe assert is what you should consider.  Static factory classes have an empty private constructor which is a clear idiom IMO.

Mandy