[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-jmx-dev
Subject: Re: jmx-dev [ping][ping] Re: RFR: 8004926 sun/management/jmxremote/bootstrap/CustomLauncherTest.sh o
From: David Holmes <david.holmes () oracle ! com>
Date: 2013-10-24 22:54:49
Message-ID: 5269A539.8020401 () oracle ! com
[Download RAW message or body]
Good to go.
Thanks,
David
On 25/10/2013 12:10 AM, Jaroslav Bachorik wrote:
> Hi David,
>
> On 10.10.2013 06:41, David Holmes wrote:
> > On 9/10/2013 9:31 PM, Jaroslav Bachorik wrote:
> > > On 9.10.2013 12:23, David Holmes wrote:
> > > > Jaroslav,
> > > >
> > > > Thanks for the details description of changes - much appreciated.
> > > >
> > > > There is a lot to digest in there. :)
> > >
> > > Yep, it started as a simple fix :/
> > >
> > > >
> > > > It isn't obvious to me why these tests require a full JDK?
> > >
> > > IDK, LocalManagementTest.sh was listed as one requiring full JDK. Its
> > > requirements are the same as the ones of CustomLauncherTest.sh (now
> > > *.java) so it seemed logical to list it there too.
> >
> > Ah! Now I see it - it uses tools.jar which implies a full JDK.
> >
> > > >
> > > > I don't quite follow the libjvm lookup logic - I would expect that you
> > > > would always want to test the libjvm that is currently running - though
> > > > it is hard to determine that.
> > >
> > > I'm afraid I can't be of much assistance here - I just took what was in
> > > the *.sh version and converted it to *.java.
> >
> > Okay. I expect this will need revisiting at some point.
>
> So, does this mean "ok, go"?
>
> Thanks,
>
> -JB-
>
> >
> > Thanks,
> > David
> > -----
> >
> >
> > > -JB-
> > >
> > > >
> > > > Thanks,
> > > > David
> > > >
> > > > On 8/10/2013 9:33 PM, Jaroslav Bachorik wrote:
> > > > > On 8.10.2013 05:42, David Holmes wrote:
> > > > > > Jaroslav,
> > > > > >
> > > > > > Can you summarise the changes please? With the conversion to Java and
> > > > > > the infrastructure additions I can't tell what is actually fixing the
> > > > > > original timeout issue :)
> > > > >
> > > > > The timeout was most caused by using the same file for communication
> > > > > between java processes in more test cases. When those test cases were
> > > > > run in parallel the file got rewritten silently and some of the tests
> > > > > could end up trying to connect to incorrect port in the target
> > > > > application. I was able to reproduce the timeout by interleaving the
> > > > > test runs for CustomLauncherTest.sh and LocalManagementTest.sh and
> > > > > adding an artificial delay to CusteomLauncherTest.sh to allow
> > > > > LocalManagementTest.sh to change the port in the file.
> > > > >
> > > > > While it could be fixed by using a different file for each test case I
> > > > > took the liberty of converting the shell tests to java tests. This
> > > > > allows me to remove the communication file and, in the end, make the
> > > > > tests more robust.
> > > > >
> > > > > CustomLauncherTest.java and LocalManagementTest.java are the tests
> > > > > converted from shell to java. I decided to convert
> > > > > LocalManagementTest.sh as well because it has the same problems as the
> > > > > CustomLauncherTest.sh.
> > > > >
> > > > > The changes in the testlibrary are about introducing new methods
> > > > > allowing the tests easily start a process and wait for a certain text
> > > > > appearing in its stdout/stderr. Using these methods the caller can
> > > > > wait
> > > > > till the callee is fully initialized and eg. ready to accept
> > > > > connections.
> > > > >
> > > > > The changes in launchers make the launchers actually executable + I am
> > > > > adding a linux-amd64 launcher (I needed that one to work on the
> > > > > changes
> > > > > locally and thought it might be nice to have one more platform covered
> > > > > by the test).
> > > > >
> > > > > I've update the webrev to include changes to LocalManagementTest and
> > > > > TEST.groups (both of those tests require JDK) -
> > > > > http://cr.openjdk.java.net/~jbachorik/8004926/webrev.05
> > > > >
> > > > > -JB-
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > David
> > > > > >
> > > > > > On 8/10/2013 12:14 AM, Jaroslav Bachorik wrote:
> > > > > > > On 19.9.2013 16:33, Jaroslav Bachorik wrote:
> > > > > > > > The updated webrev:
> > > > > > > > http://cr.openjdk.java.net/~jbachorik/8004926/webrev.03
> > > > > > > >
> > > > > > > > I've moved some of the functionality to the testlibrary.
> > > > > > > >
> > > > > > > > -JB -
> > > > > > > >
> > > > > > > > On 12.9.2013 17:31, Jaroslav Bachorik wrote:
> > > > > > > > > On 09/12/2013 05:13 PM, Dmitry Samersoff wrote:
> > > > > > > > > > Jaroslav,
> > > > > > > > > >
> > > > > > > > > > CustomLauncherTest.java:
> > > > > > > > > >
> > > > > > > > > > 102: this check could be moved to switch at ll. 108
> > > > > > > > > > otherwise test fails on "sunos" and "linux" because PLATFORM
> > > > > > > > > > remains
> > > > > > > > > > unset.
> > > > > > > > > Good idea. Thanks.
> > > > > > > > >
> > > > > > > > > > 129: I would prefer don't have pattern like this one ever in
> > > > > > > > > > shell
> > > > > > > > > > script. Could you prepare a list of VM's to check and just loop
> > > > > > > > > > over
> > > > > > > > > > it?
> > > > > > > > > > It makes test better readable. Also I think nowdays we can always
> > > > > > > > > > use
> > > > > > > > > > server VM.
> > > > > > > > > I tried to mirror the original shell test as closely as
> > > > > > > > > possible. It
> > > > > > > > > would be nice if we could rely on the "server" vm only. Definitely
> > > > > > > > > more
> > > > > > > > > readable.
> > > > > > > > >
> > > > > > > > > -JB-
> > > > > > > > >
> > > > > > > > > > -Dmitry
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On 2013-09-12 18:17, Jaroslav Bachorik wrote:
> > > > > > > > > > > On 09/12/2013 10:22 AM, Jaroslav Bachorik wrote:
> > > > > > > > > > > > On 09/12/2013 10:12 AM, Chris Hegarty wrote:
> > > > > > > > > > > > > On 09/12/2013 04:45 AM, David Holmes wrote:
> > > > > > > > > > > > > > Hi Jaroslav,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > You need a copyright notice in the new file.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > As written this test can only run on a full JDK - so \
> > > > > > > > > > > > > > please add
> > > > > > > > > > > > > > it to
> > > > > > > > > > > > > > the :needs_jdk group in TEST.groups. (Does jcmd really
> > > > > > > > > > > > > > needs to
> > > > > > > > > > > > > > come
> > > > > > > > > > > > > > from the test-jdk? And use the VMOPTS passed to the \
> > > > > > > > > > > > > > test?)
> > > > > > > > > > > > > > Is there a reason this test can't run on OSX? I know it \
> > > > > > > > > > > > > > would need
> > > > > > > > > > > > > > further modification but was wondering if there is \
> > > > > > > > > > > > > > something inherent in
> > > > > > > > > > > > > > the test that makes it inapplicable to OSX.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I think the test would be a lot simpler if the jdk tests \
> > > > > > > > > > > > > > had the
> > > > > > > > > > > > > > hotspot
> > > > > > > > > > > > > > test library's process tools available. :(
> > > > > > > > > > > > > We have some, is there an obvious gap?
> > > > > > > > > > > > >
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > Hm, thanks for the info. I should have used this library
> > > > > > > > > > > > instead.
> > > > > > > > > > > >
> > > > > > > > > > > > Please, stand by for the updated webrev.
> > > > > > > > > > > I was able to get rid off the JCMD. Using the testlibrary the
> > > > > > > > > > > target
> > > > > > > > > > > application can recognize its own PID and print it to its
> > > > > > > > > > > stdout.
> > > > > > > > > > > The
> > > > > > > > > > > main application then just reads the stdout to parse the PID. \
> > > > > > > > > > > No need
> > > > > > > > > > > for JCMD any more.
> > > > > > > > > > >
> > > > > > > > > > > I could not find a way to remove the dependency on "test.jdk"
> > > > > > > > > > > system
> > > > > > > > > > > property. According to the jtreg web documentation
> > > > > > > > > > > (http://openjdk.java.net/jtreg/vmoptions.html#cmdLineOpts) a
> > > > > > > > > > > "test.java"
> > > > > > > > > > > system property should be available but in fact is not. But it
> > > > > > > > > > > seems
> > > > > > > > > > > that the testlibrary uses "test.jdk" system property too.
> > > > > > > > > > >
> > > > > > > > > > > The test does not run on OSX because nobody built the launcher
> > > > > > > > > > > binary :)
> > > > > > > > > > > I think it is a kind of DIY so I took the liberty of adding a
> > > > > > > > > > > linux-amd64 launcher while working on the test.
> > > > > > > > > > >
> > > > > > > > > > > While working with the test library I realized I was missing a
> > > > > > > > > > > crucial
> > > > > > > > > > > feature (at least for my purposes) - waiting for a certain
> > > > > > > > > > > message to
> > > > > > > > > > > appear in the stdout/stderr of the launched process. Very
> > > > > > > > > > > often I
> > > > > > > > > > > need
> > > > > > > > > > > to wait for the target process to get to certain point before
> > > > > > > > > > > the
> > > > > > > > > > > test
> > > > > > > > > > > can be allowed to continue - and the point is indicated by a
> > > > > > > > > > > message in
> > > > > > > > > > > stdout/stderr. Currently all the proc tools are designed to
> > > > > > > > > > > work in
> > > > > > > > > > > "batch" mode - the whole stdout/stderr is captured in strings
> > > > > > > > > > > and
> > > > > > > > > > > analyzed after the target process died - and are not suitable
> > > > > > > > > > > for
> > > > > > > > > > > this
> > > > > > > > > > > kind of usage.
> > > > > > > > > > >
> > > > > > > > > > > Webrev: http://cr.openjdk.java.net/~jbachorik/8004926/webrev.01
> > > > > > > > > > >
> > > > > > > > > > > > -JB-
> > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > -Chris.
> > > > > > > > > > > > >
> > > > > > > > > > > > > > David
> > > > > > > > > > > > > > -----
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On 12/09/2013 1:39 AM, Jaroslav Bachorik wrote:
> > > > > > > > > > > > > > > Please, review the patch for an intermittently failing \
> > > > > > > > > > > > > > > test.
> > > > > > > > > > > > > > > The test is a shell test, using files for the \
> > > > > > > > > > > > > > > interprocess synchronization. This leads to \
> > > > > > > > > > > > > > > intermittent failures.
> > > > > > > > > > > > > > > In order to fix this the test is rewritten in Java - \
> > > > > > > > > > > > > > > the original
> > > > > > > > > > > > > > > functionality and outputs should be 100% preserved. The
> > > > > > > > > > > > > > > patch is
> > > > > > > > > > > > > > > unfortunately a bit difficult to follow since there is \
> > > > > > > > > > > > > > > no similarity
> > > > > > > > > > > > > > > between the *.sh and *.java file so one needs to go \
> > > > > > > > > > > > > > > through the
> > > > > > > > > > > > > > > new
> > > > > > > > > > > > > > > source in whole.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > The changes in "launcher" files are all about adding
> > > > > > > > > > > > > > > permissions to
> > > > > > > > > > > > > > > execute (0755) and as such the webrev shows no \
> > > > > > > > > > > > > > > differences.
> > > > > > > > > > > > > > > Thanks,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Issue : JDK-8004926
> > > > > > > > > > > > > > > Webrev :
> > > > > > > > > > > > > > > http://cr.openjdk.java.net/~jbachorik/8004926/webrev.00
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > -JB-
> > > > > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > >
> > >
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic