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

Daniel Fuchs daniel.fuchs at oracle.com
Tue Oct 20 15:44:28 UTC 2015

Hi Mandy,

All feedback integrated - except those discussed below.

public URL content has been refreshed: http://cr.openjdk.java.net/~dfuchs/8046565/proto.02/webrev/index.html http://cr.openjdk.java.net/~dfuchs/8046565/proto.02/specdiff-api/overview-summary.html

sandbox branch JDK-8046565-branch updated as well.

Please find answers to some of the points you raised:

On 20/10/15 09:10, Mandy Chung wrote:

On Oct 19, 2015, at 11:18 AM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:

webrev: http://cr.openjdk.java.net/~dfuchs/8046565/proto.02/webrev/index.html

[...]

LoggerFinderLoader.java

110 (PrivilegedAction<ServiceLoader>) - can this use diamond operation (PrivilegedAction<>)?

jdk/src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java:110: error: illegal start of type (PrivilegedAction<>)

sun/management/ManagementFactoryHelper.java

172 public interface LoggingMXBean 173 extends PlatformLoggingMXBean, java.util.logging.LoggingMXBean { 174 }

Was there a comment related to that? This is a thorn that will prevent java.management from not requiring java.logging.

We may be able to workaround it if we change java.management to use System.Logger directly - then we could perhaps revert the require clause and have java.logging requires javax.management instead (so that j.u.l.LoggingMXBean can extend j.l.PlatformLoggingMXBean) but I'm not sure we would be in a better situation. Or we could possibly move that to jdk.management. Maybe. Anyway - nothing we can do here - this will have to be examined later.

LazyLoggers.java 68 this(Objects.requireNonNull(loggerSupplier), 69 (Void)null); 130 this(Objects.requireNonNull(name), Objects.requireNonNull(factories), 131 Objects.requireNonNull(caller), null); I would expect Objects.requireNonNull on the constructor initializes all the fields. But the above lines checks non-null before calling this(….). Does the other constructor allow null parameters? Why not moving Objects.requireNonNull to the other constructor?

Just a way to throw the exception before the super constructor is called. The other constructor is private. When we reach it the check will already have been made. We could check again but for what purpose?

BootstrapLogger.DetectBackend looks like have some duplication as LoggerFinderLoader. - does it make sense to merge them?

I will consider it. I'd prefer not to.

923 boolean cleared; - should that be volatile field?

No. It's accessed only within a synchronized block.

java.util.logging.LogManager.demandLoggerFor - I think this method is intended for the LoggerFinder provider to use. Perhaps we should add a note in the javadoc to say that.

Would you have something to suggest? I was thinking that this method could also provide a solution for JDK-8023163.

SimpleConsoleLogger.skipLoggingFrame and another place is finding the caller information. JEP 259 may be useful for stack walking with filtering.

I skimmed through the tests and nothing pops up. There are multiple copies of AccessSystemLogger that can be shared as I comment last time.

AccessSystemLogger is a trick to have a class in the BCL so that it gets a system logger as a platform class. I have a local workspace where I ported 8046565 to jake (not replacing 'Class<?> caller' with 'Module caller' yet to avoid merging issue while integrating review feedback), but last time I checked all these tests were passing on jake without modification. I know that we will have additional/different ways of unit testing unexported internal APIs with the module system, so I'd rather revisit that after the initial push.

I can add a subtask to JDK-8046565 so that your input doesn't get forgotten.

best regards,

-- daniel

Mandy

More information about the core-libs-dev mailing list