RFR (S): 8022229: Intermittent test failures in sun/tools/jstatd

Staffan Larsen staffan.larsen at oracle.com
Wed Oct 30 08:01:30 PDT 2013 

Looks good.

I will sponsor the commit.

Thanks,
/Staffan

On 30 okt 2013, at 14:31, Yekaterina Kantserova <yekaterina.kantserova at oracle.com> wrote:

> I've forgot to mention one more fix. I've renamed JstatGcutilParser to JstatGCUtilParser (a tip from Erik), to follow standards in testlibrary (JDKToolLauncher, not JdkToolLauncher).
> 
> Here is a new webrev:
> http://cr.openjdk.java.net/~ykantser/8022229/webrev.05/
> 
> The tests have passed in JPRT run:
> http://sthjprt.se.oracle.com/archives/2013/10/2013-10-30-120951.ykantser.tl/
>  
> The attached patch is ready to be pushed.
> 
> 
> Staffan, Erik, Jaroslav, Peter: thanks a lot for your time and patient,
> Katja
> 
> 
> 
> On 10/30/2013 10:27 AM, Yekaterina Kantserova wrote:
>> 
>> http://cr.openjdk.java.net/~ykantser/8022229/webrev.04/
>> 
>> Fixed your points and changed ./test/sun/tools/jstatd/TestJstatdUsage.java to use JDKToolLauncher instead of JDKFinder.
>> 
>> Thanks,
>> Katja
>> 
>> 
>> On 10/28/2013 07:02 PM, Staffan Larsen wrote:
>>> Rename JstatdTest.getToolOptions() to JstatdTest.getDestination().
>>> 
>>> nit: JstatdTest.cleanUpThread() shouldn't be static.
>>> 
>>> Thanks,
>>> /Staffan
>>> 
>>> On 28 okt 2013, at 17:12, Yekaterina Kantserova <yekaterina.kantserova at oracle.com> wrote:
>>> 
>>>> Hi,
>>>>  
>>>> http://cr.openjdk.java.net/~ykantser/8022229/webrev.03/
>>>> 
>>>> Moved ToolConfig functionality to JstatdTest (renamed JstatdHelper) and deleted ToolConfig. The tests are also changed a little:
>>>> 
>>>> public class TestJstatdDefaults {
>>>> 
>>>> public static void main(String[] args) throws Throwable {
>>>> JstatdTest test = new JstatdTest();
>>>> test.doTest();
>>>> }
>>>> 
>>>> }
>>>> 
>>>> Hope it looks better now.
>>>> 
>>>> Thanks,
>>>> Katja
>>>> 
>>>> ----- Ursprungligt meddelande -----
>>>> Från: staffan.larsen at oracle.com
>>>> Till: yekaterina.kantserova at oracle.com
>>>> Kopia: serviceability-dev at openjdk.java.net, david.holmes at oracle.com, jaroslav.bachorik at oracle.com,christian.tornqvist at oracle.com, hotspot-dev at openjdk.java.net, mattis.castegren at oracle.com
>>>> Skickat: måndag, 28 okt 2013 12:57:38 GMT +01:00 Amsterdam/Berlin/Bern/Rom/Stockholm/Wien
>>>> Ämne: Re: RFR (S): 8022229: Intermittent test failures in sun/tools/jstatd
>>>> 
>>>> This looks a lot better. Just few more comments:
>>>> 
>>>> ToolConfig.java
>>>>   L72 - "getToolOptins" -> "getToolOptions"
>>>>   L35 - why is port stored as a String? Also, could be renamed to rmiRegistryPort to clarify what it is for.
>>>> 
>>>>   Why are some tool-specific options added in getToolOptins() and some in the getJpsCmd()/getJstatdCmd()/... methods? Maybe changing getToolOptins() to only return the host part of the options and moving the rest of the setup to getJpsCmd()/getJstatdCmd()/...?
>>>> 
>>>> 
>>>> Thanks,
>>>> /Staffan
>>>> 
>>>> 
>>>> On 24 okt 2013, at 15:57, Yekaterina Kantserova <yekaterina.kantserova at oracle.com> wrote:
>>>> 
>>>> Hi,
>>>> 
>>>> I've done following big changes after the Staffan's review:
>>>> - ported JDKToolLauncher, JDKToolFinder, Plattform from hotspot testlibrary (http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot) for re-using in ToolConfig
>>>> - changed "verifiy output form jps, jstat"-methods to ignore warnings from VM
>>>> - tools will be launched without -vmoptions
>>>> 
>>>> plus made fixes for all minor comments.
>>>> 
>>>> If it's a good idea to push testlibrary changes separately I can make a separate patch for them.
>>>> 
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~ykantser/8022229/webrev.02/
>>>> 
>>>> Primal bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8022229
>>>> 
>>>> Similar bugs:
>>>> https://bugs.openjdk.java.net/browse/JDK-8019630
>>>> https://bugs.openjdk.java.net/browse/JDK-6636094
>>>> https://bugs.openjdk.java.net/browse/JDK-6543979
>>>> 
>>>> Thanks,
>>>> Katja
>>>> 
>>>> 
>>>> 
>>>> On 10/23/2013 12:55 PM, Staffan Larsen wrote:
>>>> test/lib/testlibrary/jdk/testlibrary/Asserts.java
>>>>   Same code as already exists in the hotspot testlibrary.
>>>>   No further comments.
>>>> 
>>>> 
>>>> test/lib/testlibrary/jdk/testlibrary/ProcessThread.java
>>>>   L33: stating -> starting
>>>>   L66: staring -> starting
>>>> 
>>>> 
>>>> test/lib/testlibrary/jdk/testlibrary/TestThread.java
>>>>   This code comes from an internal test library.
>>>>   No further comments.
>>>> 
>>>> 
>>>> test/lib/testlibrary/jdk/testlibrary/Utils.java
>>>>   No comments.
>>>> 
>>>> 
>>>> test/lib/testlibrary/jdk/testlibrary/XRun.java
>>>>   This code comes from an internal test library.
>>>>   No further comments.
>>>> 
>>>> 
>>>> test/sun/tools/jstatd/JstatGcutilParser.java
>>>>   The parse() method could be made more robust by discarding any lines that come before the header lines. This can happen if the JVM outputs a warning message for some reason before running jstat.
>>>>   
>>>>   I don't like the special-case for the CCS column in verify() - took me a while to find it. Should we add a new enum called PERCENTAGE_OR_DASH to handle that instead?
>>>> 
>>>>   L67: getType() should be private.
>>>> 
>>>> 
>>>> test/sun/tools/jstatd/JstatdHelper.java:
>>>>   L54: Does verifyJpsOutput() work correctly with output of the form:
>>>> 
>>>> 1234 -- main class information unavailable
>>>> 1235 -- process information unavailable
>>>> 
>>>>   Also: same comment here as for JstatGcutilParser.java: sometimes the JVM outputs warning messages before the output from the tool.
>>>> 
>>>>   L46: "attach" is probably not the best way to describe the operation of jps. It does not attach to the process, it merely lists the running processes. Perhaps runJps() is a better name?
>>>> 
>>>> 
>>>> test/sun/tools/jstatd/TestJstatdDefaults.java
>>>> test/sun/tools/jstatd/TestJstatdExternalRegistry.java
>>>> test/sun/tools/jstatd/TestJstatdPort.java
>>>> test/sun/tools/jstatd/TestJstatdPortAndServer.java
>>>> test/sun/tools/jstatd/TestJstatdServer.java
>>>>   No comments.
>>>> 
>>>> 
>>>> test/sun/tools/jstatd/TestJstatdUsage.java
>>>>   Same comment here as for JstatGcutilParser.java: sometimes the JVM outputs warning messages before the output from the tool.
>>>> 
>>>> 
>>>> test/sun/tools/jstatd/ToolConfig.java
>>>>   ToolConfig + Utils.get(Forward)VmOptions() seem to be doing similar things to JDKToolLauncher in the hotspot testlibrary. It is unfortunate if we have two ways to do similar things in the two different testlibraries. Would it be possible to reuse the hotspot code here instead?
>>>> 
>>>>   You have also changed the test. Previously the tools were not launched with the jtreg -vmoptions (test.vm.opts), whereas now they will be. Is this intentional? 
>>>> 
>>>> 
>>>> General comments:
>>>> 
>>>> Can't we do:
>>>>   * @library /lib/testlibrary
>>>> instead of
>>>>   * @library ../../../lib/testlibrary
>>>> ?
>>>> 
>>>> It seems that at least some of the functionality should be reused for re-writing the jstat and jps tests as well. I guess we can look at that at a later time, though.
>>>> 
>>>> While I applaud the move from shell scripts to Java code, I can't shake the feeling that the shell scripts were easier to read and follow. The Java code is much more verbose and spread out over multiple helpers and libraries, making it harder to grasp. That may be the price we have to pay, though.
>>>> 
>>>> 
>>>> Thanks,
>>>> /Staffan
>>> 
>> 
> 
> <8022229.5.patch>

More information about the hotspot-dev mailing list