[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8048710 Use ServiceLoader for the jvmstat protocols
From:       Alan Bateman <Alan.Bateman () oracle ! com>
Date:       2014-06-30 13:25:53
Message-ID: 53B16561.4020506 () oracle ! com
[Download RAW message or body]

On 30/06/2014 12:22, Staffan Larsen wrote:
> All,
> 
> The jvmstat implementation has three different protocols (file, local and rmi). \
> These protocol implementations are currently loaded with Class.forName() using a \
> specific scheme to create the class name. A more standard approach is to use \
> ServiceLoader for this instead. This also makes it easier to break out different \
> implementation classes for different packagings. 
> webrev: http://cr.openjdk.java.net/~sla/8048710/webrev.00/
> bug: https://bugs.openjdk.java.net/browse/JDK-8048710
> 
This looks good to me.

One comment on monitoredHostServiceLoader is that it specifies the class 
loader as null and so will search the system class loader. I think this 
is okay as it provides a bit of flexibility to deploy other protocols on 
the class path if needed.

One other comment is that using ServiceLoader with a security manager 
often requires additional permissions to be granted because it involve 
scanning the class path. I don't know if we have any tests that use 
jvmstat with a security manager, just something to mention in case it 
comes up.

-Alan


[Attachment #3 (text/html)]

<html>
  <head>
    <meta content="text/html; charset=ISO-8859-1"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <div class="moz-cite-prefix">On 30/06/2014 12:22, Staffan Larsen
      wrote:<br>
    </div>
    <blockquote
      cite="mid:BFBEAF9D-1438-4DF0-B907-8DB40DE2605D@oracle.com"
      type="cite">
      <pre wrap="">All,

The jvmstat implementation has three different protocols (file, local and rmi). These \
protocol implementations are currently loaded with Class.forName() using a specific \
scheme to create the class name. A more standard approach is to use ServiceLoader for \
this instead. This also makes it easier to break out different implementation classes \
for different packagings.

webrev: <a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~sla/8048710/webrev.00/">http://cr.openjdk.java.net/~sla/8048710/webrev.00/</a>
                
bug: <a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8048710">https://bugs.openjdk.java.net/browse/JDK-8048710</a>


</pre>
    </blockquote>
    This looks good to me.<br>
    <span class="new"><br>
      One comment on monitoredHostServiceLoader</span>
    <meta http-equiv="content-type" content="text/html;
      charset=ISO-8859-1">
    is that it specifies the class loader as null and so will search the
    system class loader. I think this is okay as it provides a bit of
    flexibility to deploy other protocols on the class path if needed.<br>
    <br>
    One other comment is that using ServiceLoader with a security
    manager often requires additional permissions to be granted because
    it involve scanning the class path. I don't know if we have any
    tests that use jvmstat with a security manager, just something to
    mention in case it comes up.<br>
    <br>
    -Alan<br>
    <br>
    <br>
  </body>
</html>



[prev in list] [next in list] [prev in thread] [next in thread]

Configure | About | News | Add a list | Sponsored by KoreLogic