RFR (S): 8022229: Intermittent test failures in sun/tools/jstatd
Yekaterina Kantserova
yekaterina.kantserova at oracle.com
Wed Oct 30 06:31:38 PDT 2013
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
>> <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
>>>
>>
>
More information about the hotspot-dev
mailing list