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
