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