RFR: JDK-8046565: Platform Logger API and Service

[Daniel Fuchs]

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