Looks good. I will sponsor the commit.
Thanks, /Staffan On 30 okt 2013, at 14:31, Yekaterina Kantserova <yekaterina.kantser...@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.kantser...@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.lar...@oracle.com >>>> Till: yekaterina.kantser...@oracle.com >>>> Kopia: serviceability-dev@openjdk.java.net, david.hol...@oracle.com, >>>> jaroslav.bacho...@oracle.com,christian.tornqv...@oracle.com, >>>> hotspot-...@openjdk.java.net, mattis.casteg...@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.kantser...@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>
