> On Oct 13, 2015, at 5:14 AM, Daniel Fuchs <daniel.fu...@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.SYSTEM_LOGGER_RB_NAME, 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 LOGGERFINDER_PERMISSION =
>> 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
