RFR: JDK-8046565: Platform Logger API and Service

[Daniel Fuchs]

On 14/10/15 18:13, Mandy Chung wrote:
> There are other methods having similar @throws that should be updated as well.

Yes - sorry - I meant I would do it for the method that takes Object as
well.

>> When I started working on this I added a new LoggerPermission
>> class similar to java.util.logging.Permission. I received
>> strong pushback against it and it was suggeseted I used
>> RuntimePermission instead - which suits me well.
>>
>
> I slightly prefer a specific Permission type for this.

I am reluctant to add LoggerPermission back.
Not only because I'd have to find a home for it - but also
because RuntimePermission("loggerFinder") seems appropriate
and conforms to what other service implementation do (and
it doesn't add any new class)

>>> LazyLoggers::getLazyLogger(String name, Class<?> caller)
>>> - does it need permission check? it’s currently commented out
>>
>> This class should already be protected by packageAccess checks and
>> module access checks (since it's not in an exported package).
>> So I don't think it needs the additional permission check
>> which should have been performed (if needed) by upstream code.
>> The commented code should obviously go away before pushing,
>> I left it to elicit confirmation that it was OK to leave
>> it out.
>
> Take it out then.  I was not sure if you have it commented as a question to the reviewer or not.

Thanks :-) Will do.

>>> public class LoggerWrapper<L extends Logger> extends AbstractLoggerWrapper<L>
>>>      implements Logger, PlatformLoggerBridge
>>>
>>> - AbstractLoggerWrapper implements Logger, PlatformLoggerBridge, ConfigurableLoggerBridgem.  Is "implements Logger, PlatformLoggerBridge” needed at all?
>>
>
> What I tried to point out is the class declaration.  Should LoggerWrapper simply extend AbstractLoggerWrapper (and drop the “implements….”)?  It’s minor and try to understand why you declare “implements…” explicitly that are the super interfaces of AbstractLoggerWrapper.

Oh sorry - I misunderstood your comment. That's an oversight
indeed, I will fix it.

>>> SimpleConsoleLogger.Formatting
>>> static final String FORMAT_PROP_KEY =
>>>              "java.util.logging.SimpleFormatter.format";
>>> - is this used when java.logging is not present?  It’s for the simple console logger formatting.  If so, the property name should be renamed to idk.logger.xxx?
>>
>> Yes. It's used both by java.util.logging and the SimpleConsoleLogger.
>> I think you did introduce this property - it was also used both by
>> java.util.logging and the PlatformLogger proxy - I simply
>> translated the code from the proxy to SimpleConsoleLogger.
>
> Yes I did.  It’s a standard property that java.util.logging supports.  PlatformLogger proxy used it.  I think we should look into some simple mechanism for simple console configuration when java.logging is not present (a RFE to cover it).  It would be confusing to reuse “java.util.logging.” property to configure simple console in the absence of java.logging.

It can be useful too if you want to have ms precision in
time stamps. I agree it is strange to honor this contract when
java.logging is not there.
On the other hand - it might be simpler to always honor the
contract. Let me think on it.

>> SimpleConsoleLogger are also used as temporary loggers when
>> LogManager is not initialized and has no custom configuration,
>> just as PlatformLogger used to do. That functionality was
>> simply translated from PlatformLogger to LazyLoggers/BootstrapLogger.
>
> That’s the next big thing to review.
>
>>
>>> Can you explain the difference of sun.util.logging and sun.util.logger package? I think the reason to have two different packages is to make sure that sun.util.logger not used by other modules.  What other issue to put the classes in sun.util.logging - the existing package?  I don’t have any issue to create a new package but the name is hard to differentiate.   Maybe rename sun.util.logger to jdk.internal.logger if not in sun.util.logging?
>>
>> sun.util.logging contains the 'legacy' PlatformLogger and all
>> the code needed to bridge PlatformLogger.
>> It can go away when we get to the point where nobody uses
>> PlatformLogger any more.
>>
>> :
>> In an ideal situation then in the mid-long term sun.util.logging
>> should hopefully disappear from java.base - and only
>> sun.util.logger would remain.
>>
>
> I’m glad that it’s a plan to migrate existing uses of sun.util.logging.PlatformLogger to System.Logger and ultimately PlatformLogger will go away.
>
>> Do you think it would be preferable to move all the classes
>> from sun.util.logger over to sun.util.logging?
>> The distinction between new and legacy/bridge might be less
>> clear, and there might be a little more risk of having modules
>> which already read sun.util.logging foraging inside - but it
>> could be worth it as it would mean one less new package…
>
> I think it’s better to rename the new package to jdk.internal.logger to make it easier to distinct.

OK - I will do the renaming then.

-- daniel

>
> Mandy
>