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

Wed Oct 30 02:27:41 PDT 2013

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 
> <mailto: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 <mailto:staffan.larsen at oracle.com>
>> Till:yekaterina.kantserova at oracle.com 
>> <mailto:yekaterina.kantserova at oracle.com>
>> Kopia: serviceability-dev at openjdk.java.net 
>> <mailto:serviceability-dev at openjdk.java.net>,david.holmes at oracle.com 
>> <mailto:david.holmes at oracle.com>,jaroslav.bachorik at oracle.com 
>> <mailto:jaroslav.bachorik at oracle.com>,christian.tornqvist at oracle.com 
>> <mailto:christian.tornqvist at oracle.com>, hotspot-dev at openjdk.java.net 
>> <mailto:hotspot-dev at openjdk.java.net>,mattis.castegren at oracle.com 
>> <mailto: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 
>> <mailto: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
>>
>>     plusmade fixes for all minorcomments.
>>
>>     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
>>
>>     Similarbugs:
>>     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
>>
>