RFR : JDK-8175542 - JMX: Not enough JDP packets received

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR : JDK-8175542 - JMX: Not enough JDP packets received

Amit Sapre

Hello,

 

Please review the timeout fix for this bug.

 

Bug ID : https://bugs.openjdk.java.net/browse/JDK-8175542

Webrev : http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-8175542/webrev.00/

 

Thanks,

Amit

Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8175542 - JMX: Not enough JDP packets received

Harsha Wardhana B

Hi Amit,

Increasing the timeout 'TIME_OUT_FACTOR' will increase both socket and test-case timeout. The test passed as can be seen from the log, but because of the race-condition: hasTestLivedLongEnough() checked before shouldContinue(), the test was declared failed because of time-out.

One possible fix would be to change line 80 to,

                if (shouldContinue() && hasTestLivedLongEnough())

Thanks

Harsha


On Wednesday 13 December 2017 11:03 AM, Amit Sapre wrote:

Hello,

 

Please review the timeout fix for this bug.

 

Bug ID : https://bugs.openjdk.java.net/browse/JDK-8175542

Webrev : http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-8175542/webrev.00/

 

Thanks,

Amit


Reply | Threaded
Open this post in threaded view
|

RE: RFR : JDK-8175542 - JMX: Not enough JDP packets received

Amit Sapre

Thanks Harsha for the inputs.

 

I made changes in this webrev : http://cr.openjdk.java.net/~asapre/webrev/2018/JDK-8175542/webrev.01/

 

Thanks,

Amit

 

From: Harsha Wardhana B
Sent: Wednesday, December 13, 2017 5:32 PM
To: [hidden email]
Subject: Re: RFR : JDK-8175542 - JMX: Not enough JDP packets received

 

Hi Amit,

Increasing the timeout 'TIME_OUT_FACTOR' will increase both socket and test-case timeout. The test passed as can be seen from the log, but because of the race-condition: hasTestLivedLongEnough() checked before shouldContinue(), the test was declared failed because of time-out.

One possible fix would be to change line 80 to,

                if (shouldContinue() && hasTestLivedLongEnough())

Thanks

Harsha

 

On Wednesday 13 December 2017 11:03 AM, Amit Sapre wrote:

Hello,

 

Please review the timeout fix for this bug.

 

Bug ID : https://bugs.openjdk.java.net/browse/JDK-8175542

Webrev : http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-8175542/webrev.00/

 

Thanks,

Amit

 

Reply | Threaded
Open this post in threaded view
|

RE: RFR : JDK-8175542 - JMX: Not enough JDP packets received

Amit Sapre

Hello,

 

Ping. Can somebody review this code changes ?

 

Thanks,

Amit

 

From: Amit Sapre
Sent: Wednesday, January 03, 2018 5:54 PM
To: Harsha wardhana B; [hidden email]
Subject: RE: RFR : JDK-8175542 - JMX: Not enough JDP packets received

 

Thanks Harsha for the inputs.

 

I made changes in this webrev : http://cr.openjdk.java.net/~asapre/webrev/2018/JDK-8175542/webrev.01/

 

Thanks,

Amit

 

From: Harsha Wardhana B
Sent: Wednesday, December 13, 2017 5:32 PM
To: [hidden email]
Subject: Re: RFR : JDK-8175542 - JMX: Not enough JDP packets received

 

Hi Amit,

Increasing the timeout 'TIME_OUT_FACTOR' will increase both socket and test-case timeout. The test passed as can be seen from the log, but because of the race-condition: hasTestLivedLongEnough() checked before shouldContinue(), the test was declared failed because of time-out.

One possible fix would be to change line 80 to,

                if (shouldContinue() && hasTestLivedLongEnough())

Thanks

Harsha

 

On Wednesday 13 December 2017 11:03 AM, Amit Sapre wrote:

Hello,

 

Please review the timeout fix for this bug.

 

Bug ID : https://bugs.openjdk.java.net/browse/JDK-8175542

Webrev : http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-8175542/webrev.00/

 

Thanks,

Amit

 

Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8175542 - JMX: Not enough JDP packets received

David Holmes
Hi Amit, Harsha,

On 11/01/2018 4:24 PM, Amit Sapre wrote:
> Hello,
>
> Ping. Can somebody review this code changes ?

I took a look. If I understand correctly the way the test works is that
it sets up a socket timeout of "timeout" - to be applied to the
receive() call - and then allows 20% on top of that value for the
overall execution time (between the recording of the start time and the
check of hasTestLivedLongEnough()). The problem we see is that if the
receive() and related processing takes us past the end deadline then we
report a timeout, even though we actually managed to complete the test
logic successfully. The fix simply checks for success before checking
for the overall timeout - that seems reasonable.

The only concern I'd have - and there's no way to completely mitigate
this - is that if the logging+connection+creation+receive combined take
a ridiculously long time (but within the overall jtreg timeout) then we
won't notice as long as the receive+packet-processing completes okay.

Two things I'd do in the future (if this starts timing out again) before
changing the overall timeout-factor would be to try:

a) recording the start time just before the loop, instead of before the
logging and the socket connection and Datagram creation**; and/or

b) changing the timeout overhead allowance from 20% to, say, 30%.

** this has the same problem of hiding unexpected delays in the
logging+connection etc.

But the proposed fix seems adequate for now.

Thanks,
David

> Thanks,
>
> Amit
>
> *From:*Amit Sapre
> *Sent:* Wednesday, January 03, 2018 5:54 PM
> *To:* Harsha wardhana B; [hidden email]
> *Subject:* RE: RFR : JDK-8175542 - JMX: Not enough JDP packets received
>
> Thanks Harsha for the inputs.
>
> I made changes in this webrev :
> http://cr.openjdk.java.net/~asapre/webrev/2018/JDK-8175542/webrev.01/
>
> Thanks,
>
> Amit
>
> *From:*Harsha Wardhana B
> *Sent:* Wednesday, December 13, 2017 5:32 PM
> *To:* [hidden email]
> <mailto:[hidden email]>
> *Subject:* Re: RFR : JDK-8175542 - JMX: Not enough JDP packets received
>
> Hi Amit,
>
> Increasing the timeout 'TIME_OUT_FACTOR' will increase both socket and
> test-case timeout. The test passed as can be seen from the log, but
> because of the race-condition: hasTestLivedLongEnough() checked before
> shouldContinue(), the test was declared failed because of time-out.
>
> One possible fix would be to change line 80 to,
>
>                  if (shouldContinue() && hasTestLivedLongEnough())
>
> Thanks
>
> Harsha
>
> On Wednesday 13 December 2017 11:03 AM, Amit Sapre wrote:
>
>     Hello,
>
>     Please review the timeout fix for this bug.
>
>     Bug ID : https://bugs.openjdk.java.net/browse/JDK-8175542
>
>     Webrev :
>     http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-8175542/webrev.00/ <http://cr.openjdk.java.net/%7Easapre/webrev/2017/JDK-8175542/webrev.00/>
>
>     Thanks,
>
>     Amit
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR : JDK-8175542 - JMX: Not enough JDP packets received

Harsha Wardhana B
In reply to this post by Amit Sapre

Hi Amit,

Looks okay to me.

Thanks

Harsha


On Wednesday 03 January 2018 05:54 PM, Amit Sapre wrote:

Thanks Harsha for the inputs.

 

I made changes in this webrev : http://cr.openjdk.java.net/~asapre/webrev/2018/JDK-8175542/webrev.01/

 

Thanks,

Amit

 

From: Harsha Wardhana B
Sent: Wednesday, December 13, 2017 5:32 PM
To: [hidden email]
Subject: Re: RFR : JDK-8175542 - JMX: Not enough JDP packets received

 

Hi Amit,

Increasing the timeout 'TIME_OUT_FACTOR' will increase both socket and test-case timeout. The test passed as can be seen from the log, but because of the race-condition: hasTestLivedLongEnough() checked before shouldContinue(), the test was declared failed because of time-out.

One possible fix would be to change line 80 to,

                if (shouldContinue() && hasTestLivedLongEnough())

Thanks

Harsha

 

On Wednesday 13 December 2017 11:03 AM, Amit Sapre wrote:

Hello,

 

Please review the timeout fix for this bug.

 

Bug ID : https://bugs.openjdk.java.net/browse/JDK-8175542

Webrev : http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-8175542/webrev.00/

 

Thanks,

Amit

 


Reply | Threaded
Open this post in threaded view
|

RE: RFR : JDK-8175542 - JMX: Not enough JDP packets received

Amit Sapre
In reply to this post by David Holmes
Thanks Harsha & David for the review.

I agree that this fix may not address all the other scenarios which you mentioned. Thanks for bringing it.
This fix is an attempt for making sure that tests reports pass if desired number of packets are received.

David,
I will added all your suggestions to this bug  for further reference, in case we hit another timeout issue.

Thanks,
Amit

> -----Original Message-----
> From: David Holmes
> Sent: Thursday, January 11, 2018 12:39 PM
> To: Amit Sapre; Harsha wardhana B; [hidden email]
> Subject: Re: RFR : JDK-8175542 - JMX: Not enough JDP packets received
>
> Hi Amit, Harsha,
>
> On 11/01/2018 4:24 PM, Amit Sapre wrote:
> > Hello,
> >
> > Ping. Can somebody review this code changes ?
>
> I took a look. If I understand correctly the way the test works is that it sets up
> a socket timeout of "timeout" - to be applied to the
> receive() call - and then allows 20% on top of that value for the overall
> execution time (between the recording of the start time and the check of
> hasTestLivedLongEnough()). The problem we see is that if the
> receive() and related processing takes us past the end deadline then we
> report a timeout, even though we actually managed to complete the test
> logic successfully. The fix simply checks for success before checking for the
> overall timeout - that seems reasonable.
>
> The only concern I'd have - and there's no way to completely mitigate this - is
> that if the logging+connection+creation+receive combined take a ridiculously
> long time (but within the overall jtreg timeout) then we won't notice as long
> as the receive+packet-processing completes okay.
>
> Two things I'd do in the future (if this starts timing out again) before changing
> the overall timeout-factor would be to try:
>
> a) recording the start time just before the loop, instead of before the logging
> and the socket connection and Datagram creation**; and/or
>
> b) changing the timeout overhead allowance from 20% to, say, 30%.
>
> ** this has the same problem of hiding unexpected delays in the
> logging+connection etc.
>
> But the proposed fix seems adequate for now.
>
> Thanks,
> David
>
> > Thanks,
> >
> > Amit
> >
> > *From:*Amit Sapre
> > *Sent:* Wednesday, January 03, 2018 5:54 PM
> > *To:* Harsha wardhana B; [hidden email]
> > *Subject:* RE: RFR : JDK-8175542 - JMX: Not enough JDP packets
> > received
> >
> > Thanks Harsha for the inputs.
> >
> > I made changes in this webrev :
> > http://cr.openjdk.java.net/~asapre/webrev/2018/JDK-
> 8175542/webrev.01/
> >
> > Thanks,
> >
> > Amit
> >
> > *From:*Harsha Wardhana B
> > *Sent:* Wednesday, December 13, 2017 5:32 PM
> > *To:* [hidden email]
> > <mailto:[hidden email]>
> > *Subject:* Re: RFR : JDK-8175542 - JMX: Not enough JDP packets
> > received
> >
> > Hi Amit,
> >
> > Increasing the timeout 'TIME_OUT_FACTOR' will increase both socket and
> > test-case timeout. The test passed as can be seen from the log, but
> > because of the race-condition: hasTestLivedLongEnough() checked before
> > shouldContinue(), the test was declared failed because of time-out.
> >
> > One possible fix would be to change line 80 to,
> >
> >                  if (shouldContinue() && hasTestLivedLongEnough())
> >
> > Thanks
> >
> > Harsha
> >
> > On Wednesday 13 December 2017 11:03 AM, Amit Sapre wrote:
> >
> >     Hello,
> >
> >     Please review the timeout fix for this bug.
> >
> >     Bug ID : https://bugs.openjdk.java.net/browse/JDK-8175542
> >
> >     Webrev :
> >
> > http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-
> 8175542/webrev.00/
> > <http://cr.openjdk.java.net/%7Easapre/webrev/2017/JDK-
> 8175542/webrev.0
> > 0/>
> >
> >     Thanks,
> >
> >     Amit
> >