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

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands
From:       Jini George <jini.george () oracle ! com>
Date:       2017-12-13 16:56:04
Message-ID: 1eea40cb-74d1-cb8f-0558-0ece0737d20e () oracle ! com
[Download RAW message or body]

Thank you, Serguei. I have the modified webrev after addressing the 
comments at:

http://cr.openjdk.java.net/~jgeorge/8192985/webrev.02/

jstackOutput could be null if there are attach permission issues.

Thanks,
Jini.


On 12/13/2017 4:44 AM, serguei.spitsyn@oracle.com wrote:
> Hi Jini,
> 
> Some minor comments:
> http://cr.openjdk.java.net/~jgeorge/8192985/webrev.01/test/hotspot/jtreg/serviceability/sa/ClhsdbInspect.java.html \
>  
> 
> I'd suggest to unsplit some lines (check if same applies to other two 
> tests):
> 
> 56             String jstackOutput =
> 57                 test.run(theApp.getPid(), cmds, null, null);
> ...
> 
> 78                             addressString =
> 79                                 (token.replace("<", 
> "")).replace(">", "");
> 
> No need for brackets around the token.replace.
> 
> Need a space after 'for':
> 
> 70                 for(String key: tokensMap.keySet()) {
> 
> 
> 
> Q: In what condition the jstackOutput can be null?
> 
> 59             if (jstackOutput != null) {
> 
> A suggestion is to use the same approach as in the ClhsdbScanOops test:
> 
> 60             if (universeOutput == null) {
> 61                 // Output could be null due to attach permission 
> issues
> 62                 // and if we are skipping this.
> 63                 LingeredApp.stopApp(theApp);
> 64                 return;
> 65             }
> 
> 
> Otherwise, the fix looks good to me.
> 
> Thanks,
> Serguei
> 
> 
> 
> On 12/12/17 02:38, Jini George wrote:
> > Thank you, Sharath. I have a modified webrev at:
> > 
> > http://cr.openjdk.java.net/~jgeorge/8192985/webrev.01/
> > 
> > Could a Reviewer also please take a look at it ?
> > 
> > Thanks,
> > Jini.
> > 
> > On 12/11/2017 3:41 PM, Sharath Ballal wrote:
> > > Hi Jini,
> > > Looks Good. Some nits:
> > > 
> > > http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/test/hotspot/jtreg/serviceability/sa/ClhsdbInspect.java.html \
> > >  
> > > 
> > > Since you are not passing any new VM options, the following lines
> > > 
> > > 28         import jdk.test.lib.Utils;
> > > 47             List<String> vmArgs = new ArrayList<String>();
> > > 48             vmArgs.addAll(Utils.getVmOptions());
> > > 49
> > > 50             theApp = new LingeredAppWithLock();
> > > 51             LingeredApp.startApp(vmArgs, theApp);
> > > 
> > > Can be replaced by
> > > 
> > > theApp = new LingeredAppWithLock();
> > > LingeredApp.startApp(null, theApp);
> > > 
> > > http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/test/hotspot/jtreg/serviceability/sa/ClhsdbScanOops.java.html \
> > >  
> > > 
> > > 41     public static void testWithGcType
> > > If you are not planning on this method being called from elsewhere, 
> > > you can make it private.
> > > 
> > > Thanks,
> > > Sharath
> > > 
> > > 
> > > -----Original Message-----
> > > From: Jini George
> > > Sent: Friday, December 08, 2017 12:33 PM
> > > To: serviceability-dev@openjdk.java.net
> > > Subject: RFR: JDK-8192985: SA: Test cases for the clhsdb 'inspect', 
> > > 'scanoops' and 'printas' commands
> > > 
> > > Hello,
> > > 
> > > Requesting reviews for:
> > > 
> > > JBS Id: https://bugs.openjdk.java.net/browse/JDK-8192985
> > > Webrev: http://cr.openjdk.java.net/~jgeorge/8192985/webrev.00/
> > > 
> > > These are the new test cases for the following clhsdb commands:
> > > 1. inspect
> > > 2. scanoops
> > > 3. printas
> > > 
> > > These tests have been verified through the Mach5 and jprt systems.
> > > 
> > > Thanks,
> > > Jini.
> > > 
> 


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