RFR: JDK-8046565: Platform Logging API and Service (original) (raw)

Mandy Chung mandy.chung at oracle.com
Tue Oct 13 16:53:26 UTC 2015

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.SYSTEMLOGGERRBNAME, 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 LOGGERFINDERPERMISSION = 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

More information about the core-libs-dev mailing list