[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java fails: FAILED: queryMBeans se
From: Daniil Titov <daniil.x.titov () oracle ! com>
Date: 2020-04-27 19:37:08
Message-ID: 35BEAC7E-F4D0-4C5A-B19F-CB21738B9283 () oracle ! com
[Download RAW message or body]
Thank you, Chris and Serguei, for reviewing this change.
I will fix typos before pushing in the repository.
Best regards,
Daniil
On 4/27/20, 12:12 PM, "Chris Plummer" <chris.plummer@oracle.com> wrote:
Hi Daniil,
83 // names2, and names3 will have different size. Repeat
the test in this case.
Should be "sizes". There are a few instances of this in the comments
that need to be changed.
Looks good otherwise. I don't need to see another webrev.
thanks,
Chris
On 4/25/20 9:11 AM, Daniil Titov wrote:
> Hi Serguei,
>
> Please review a new version of the webrev [1] that changes the condition
> as you suggested. Please note then in both cases we need break from the loop : \
the case when > it is a first attempt and the conditions are met and the case when \
it is a second attempt. >
>
> [1] http://cr.openjdk.java.net/~dtitov/8242239/webrev.03
> [2] https://bugs.openjdk.java.net/browse/JDK-8242239
>
> Thank you,
> Daniil
>
>
> From: "serguei.spitsyn@oracle.com" <serguei.spitsyn@oracle.com>
> Date: Saturday, April 25, 2020 at 12:06 AM
> To: Daniil Titov <daniil.x.titov@oracle.com>, Chris Plummer \
<chris.plummer@oracle.com>, serviceability-dev <serviceability-dev@openjdk.java.net> \
> Subject: Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java \
> fails: FAILED: queryMBeans sets same
>
> Hi Daniil,
>
> Thank you for the update.
>
> On 4/24/20 22:22, Daniil Titov wrote:
> Hi Chris and Serguei,
>
> Please review a new version of the fix [1] that makes the tests try to repeat \
the check only once. >
> Regarding the question Serguei asked:
>
> 97 while(true) {
> 113 if (mbeanCount * 2 == counterCount || isSecondAttempt) {
> 114 assertEquals(mbeanCount * 2, counterCount);
> I doubt, the assert is really needed.
> we need the assert here to make the test fail in case if it is a second attempt \
and the conditions are not met. >
> A space is still needed in "while(true)."
> 111 if (mbeanCount * 2 == counterCount || isSecondAttempt) {
> 112 assertEquals(mbeanCount * 2, counterCount);
> 113 break;
> 114 }
> The code above is a little confusing as we have to logically separate the \
assert and break cases. > Would something like below be more straightforward?:
> if (mbeanCount * 2 == counterCount) {
> break;
> }
> if (isSecondAttempt) {
> assertEquals(mbeanCount * 2 != counterCount);
> }
>
> Otherwise, the update looks good.
>
> Thanks,
> Serguei
>
>
> [1] http://cr.openjdk.java.net/~dtitov/8242239/webrev.02/
> [2] https://bugs.openjdk.java.net/browse/JDK-8242239
>
> Thank you,
> Daniil
>
>
> From: serviceability-dev mailto:serviceability-dev-bounces@openjdk.java.net on \
behalf of Daniil Titov mailto:daniil.x.titov@oracle.com > Date: Friday, April 24, \
2020 at 2:08 PM > To: Chris Plummer mailto:chris.plummer@oracle.com, \
mailto:serguei.spitsyn@oracle.com mailto:serguei.spitsyn@oracle.com, \
serviceability-dev mailto:serviceability-dev@openjdk.java.net > Subject: Re: RFR: \
8242239: [Graal] javax/management/generified/GenericTest.java fails: FAILED: \
queryMBeans sets same >
> Hi Chris,
>
> I agree with you. I will change the test to retry just once.
>
> Thank you,
> Daniil
>
>
> From: Chris Plummer mailto:chris.plummer@oracle.com
> Date: Friday, April 24, 2020 at 1:27 PM
> To: Daniil Titov mailto:daniil.x.titov@oracle.com, \
mailto:serguei.spitsyn@oracle.com mailto:serguei.spitsyn@oracle.com, \
serviceability-dev mailto:serviceability-dev@openjdk.java.net > Subject: Re: RFR: \
8242239: [Graal] javax/management/generified/GenericTest.java fails: FAILED: \
queryMBeans sets same >
> Hi Daniil,
>
> I think a retry count more in line with our current expectations would make \
more sense since it is deterministic how many retries are might actually be needed. \
You just used a large number in case more "lazy-registered" mbeans are added in the \
future, but if this is the case then maybe even 10 is not enough. >
> thanks,
>
> Chris
>
> On 4/24/20 12:36 PM, Daniil Titov wrote:
> Hi Serguei and Chris,
>
> Thank you for your comments.
>
> I wanted to make the fix more generic and not limit it just to Graal \
management bean. In this case we could expect that after the first retry is triggered \
by Graal MBean registration some other "lazy-registered" MBean got registered and the \
test might fail. To avoid this we need to allow test to make at least several retry \
attempts before failing. If number 10 looks as too high we could make it 5 or even \
3. This should not affect the test run-time unless the test starts failing for some \
other reason than we expect. In the new webrev I will also correct the iteration \
counting (as Chris mentioned) and fix typos. >
> Thanks again,
> Daniil
>
> From: mailto:serguei.spitsyn@oracle.com mailto:serguei.spitsyn@oracle.com
> Date: Friday, April 24, 2020 at 11:48 AM
> To: Chris Plummer mailto:chris.plummer@oracle.com, Daniil Titov \
mailto:daniil.x.titov@oracle.com, serviceability-dev \
mailto:serviceability-dev@openjdk.java.net > Subject: Re: RFR: 8242239: [Graal] \
javax/management/generified/GenericTest.java fails: FAILED: queryMBeans sets same > \
> Hi Daniil,
>
> Besides what Chris is pointed out the fix looks good.
>
> Minor:
> 97 while(true) {
> 113 if(mbeanCount * 2 == counterCount || retryCounter++ > \
MAX_RETRY_ATTEMPT) { > 114 assertEquals(mbeanCount * 2, \
counterCount); > Space is missed in while and if.
> I doubt, the assert is really needed.
> 96 // is running ( e.g. Graal MBean). In this case just retry the \
test. > Extra space before "e.g.".
>
> Thanks,
> Serguei
>
>
> On 4/24/20 11:30, Chris Plummer wrote:
> Hi Daniil,
>
> 84 // If new MBean (e.g. Graal MBean) is registred while the \
test is running names1, > 106 // If new MBean (e.g. Graal MBean) is \
registred while the test is running mbeans1, >
> registred -> registered
> ',' after "running"
>
> Just wondering how you chose 10 as the number of retries. Seems excessive. \
Shouldn't the problem turn up at most 1 time and therefore only 1 retry is needed. >
> 76 int counter = 0;
> 86 if (sameSize(names1, names2, names3) || counter++ > \
MAX_RETRY_ATTEMPTS) { >
> The way the checks are done you will actually end up retrying \
MAX_RETRY_ATTEMPTS+1 times. For example, if MAX_RETRY_ATTEMPTS is 1, first time \
through the loop counter is 0 so a retry is allowed. Second time through the loop \
counter is 1, so a retry is allowed again. >
> thanks,
>
> Chris
>
> On 4/18/20 11:30 AM, Daniil Titov wrote:
>
>
>
> Please review the change that fixes the failure of \
javax/management/generified/GenericTest.java and > \
javax/management/query/CustomQueryTest.java tests when Graal is on. >
> The tests checks that calls of management API are consistent and return the \
same set of MBeans. However, > when Graal is on the Graal MBean might be registered \
between the calls that in turn makes the results > inconsistent and the tests fail.
>
> The fix makes the test aware that some MBean might be registered while the \
checks run and if it happens the tests repeat the check. >
> Testing : Mach5 tests with Graal on and tier1-tier3 tests passed.
>
> [1] http://cr.openjdk.java.net/~dtitov/8242239/webrev.01/
> [2] https://bugs.openjdk.java.net/browse/JDK-8242239
>
> Thanks,
> Daniil
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic