Hi Jaroslav, Shanliang Jiang Thank you very much for the review.
Following the comments from Shanliang Jiang on https://bugs.openjdk.java.net/browse/JDK-8141591 could it be possible to make, instead, a fix in the jdk source and leave the test unchanged as per the following webrev : http://cr.openjdk.java.net/~akulyakh/814591jdk/src/java.management/share/classes/com/sun/jmx/remote/internal/ClientNotifForwarder.java.udiff.html If these changes are problematic I will go for the test changes which you have already accepted. Best regards, Alexander ----- Original Message ----- From: jaroslav.bacho...@oracle.com To: alexander.kulyakh...@oracle.com, serviceability-dev@openjdk.java.net Cc: marti...@google.com Sent: Tuesday, November 10, 2015 2:10:48 PM GMT +03:00 Iraq Subject: Re: RFR: 8141591 : javax/management/remote/mandatory/threads/ExecutorTest.java fails intermittently On 9.11.2015 19:07, Alexander Kulyakhtin wrote: > Hi, > > Following the comments from Jaroslav and Martin I've changed the test > to use the unbound CountDownLatch.await() > > Since await() will wait undefinitely and thus the test, if fails, will > now fail by timeout only the code has been additionally simplified to > reflect that. > > Please, find the updated webrev at: > > CR: https://bugs.openjdk.java.net/browse/JDK-8141591 > RFR: > http://cr.openjdk.java.net/~akulyakh/8141591_03/test/javax/management/remote/mandatory/threads/ExecutorTest.java.udiff.html Looks good! -JB- > > Best regards, > Alexander > > ----- Original Message ----- > From: marti...@google.com > To: jaroslav.bacho...@oracle.com > Cc: alexander.kulyakh...@oracle.com, serviceability-dev@openjdk.java.net > Sent: Monday, November 9, 2015 7:53:08 PM GMT +03:00 Iraq > Subject: Re: RFR: 8141591 : > javax/management/remote/mandatory/threads/ExecutorTest.java fails > intermittently > > The traditional way is to have all the worker tasks count down a latch > when they're started and have the master thread wait on that before > proceeding. > > You can use test.timeout.factor to do a scaled timed wait. > > On Mon, Nov 9, 2015 at 8:03 AM, Jaroslav Bachorik > <jaroslav.bacho...@oracle.com <mailto:jaroslav.bacho...@oracle.com>> wrote: > > Hi Alexander, > > On 9.11.2015 16:40, Alexander Kulyakhtin wrote: > > Hi, > > Could you, please, review this test-only fix: > > CR: https://bugs.openjdk.java.net/browse/JDK-8141591 > Webrev: > > http://cr.openjdk.java.net/~akulyakh/8141591_01/test/javax/management/remote/mandatory/threads/ExecutorTest.java.udiff.html > > Before the fix, it was possible that we shut down the executor > before all the jobs have been submitted. This resulted in > RejectedExecutionException. > We are modifying the test to wait on CountDownLatch untill all > the jobs have completed their execute() > > > On L175 you should be using the unbound version of await(). It would > be better to let the harness deal with the timeout. > > Otherwise it looks good. > > Cheers, > > -JB- > > > Best regards, > Alexander > > >
