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; serviceability-dev@openjdk.java.net
*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:* serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@openjdk.java.net>
*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
