RFR: 8242239: [Graal] javax/management/generified/GenericTest.java fails: FAILED: queryMBeans sets same

Sat Apr 25 16:11:26 UTC 2020

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 at oracle.com" <serguei.spitsyn at oracle.com>
Date: Saturday, April 25, 2020 at 12:06 AM
To: Daniil Titov <daniil.x.titov at oracle.com>, Chris Plummer <chris.plummer at oracle.com>, serviceability-dev <serviceability-dev at 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 at openjdk.java.net on behalf of Daniil Titov mailto:daniil.x.titov at oracle.com
Date: Friday, April 24, 2020 at 2:08 PM
To: Chris Plummer mailto:chris.plummer at oracle.com, mailto:serguei.spitsyn at oracle.com mailto:serguei.spitsyn at oracle.com, serviceability-dev mailto:serviceability-dev at 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 at oracle.com
Date: Friday, April 24, 2020 at 1:27 PM
To: Daniil Titov mailto:daniil.x.titov at oracle.com, mailto:serguei.spitsyn at oracle.com mailto:serguei.spitsyn at oracle.com, serviceability-dev mailto:serviceability-dev at 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 at oracle.com mailto:serguei.spitsyn at oracle.com
Date: Friday, April 24, 2020 at 11:48 AM
To: Chris Plummer mailto:chris.plummer at oracle.com, Daniil Titov mailto:daniil.x.titov at oracle.com, serviceability-dev mailto:serviceability-dev at 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 