RFR of 8194284: CheckRegisterInLog.java fails with java.lang.RuntimeException: CheckRegisterInLog got exception timeout 6480000ms out of range

[David Holmes]

Hi Hamlin,

On 19/01/2018 4:28 PM, Hamlin Li wrote:
> 
> On 19/01/2018 2:11 PM, David Holmes wrote:
>> Hi Hamlin,
>>
>> On 19/01/2018 3:04 PM, Hamlin Li wrote:
>>> Hi Roger, David
>>>
>>> Please check the updated webrev at: 
>>> http://cr.openjdk.java.net/~mli/8194284/webrev.00/
>>
>> RMID.java:
>>
>> This comment no longer makes sense:
>>
>>       // when restart rmid, it may take more time than usual because of
>>       // "port in use" by a possible interloper (check JDK-8168975),
>>       // so need to set a longer timeout for restart.
>> !     private static final long RESTART_TIMEOUT = (long)(TIMEOUT_BASE 
>> * 0.9);
>>
>> Actually I'm not sure it originally made sense - longer than what? But 
>> as it stands RESTART_TIMEOUT is smaller than TIMEOUT_BASE so the 
>> comment really seems odd. Perhaps 8168975 will shed some light on the 
>> intent. ??
> Hi David,
> 
> The comment still makes sense.
> Before 8168975, restart() calls start() which is indeed 
> start(STARTTIME_MS), where STARTTIME_MS is 15_000L, but we found out in 
> some situation restart() needs more time than start();
> So in 8168975, restart() calls start(restartTimeout), where 
> restartTimeout is RESTART_TIMEOUT in 8194284.

Okay, then I suggest changing:

  // so need to set a longer timeout for restart.

to

  // so need to set a longer timeout than STARTTIME_MS for restart.

Thanks,
David

>>
>> The TestLibrary change looks good.
> Do you also think the comment makes sense with my explanation?
> 
> Thank you
> -Hamlin
>>
>> Thanks,
>> David
>>
>>> Thank you
>>>
>>> -Hamlin
>>>
>>>
>>> On 18/01/2018 10:33 PM, Roger Riggs wrote:
>>>> Hi Hamlin,
>>>>
>>>> The base bug is that the timeoutFactor is being applied twice, once 
>>>> in the RMID constructor
>>>> and again in computeDeadline.  It is better to cleanup the 
>>>> implementation of the test library
>>>> than to fudge the values.  Either respect the timeouts passed in 
>>>> (remove the scaling from computeDeadline)
>>>> or consistently leave it to the lower level routines.  Pick 1.
>>>>
>>>> Thanks, Roger
>>>>
>>>>
>>>> On 1/18/2018 1:59 AM, Hamlin Li wrote:
>>>>> Would you please review the following patch?
>>>>>
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8194284
>>>>>
>>>>> webrev as below.
>>>>>
>>>>>
>>>>> Thank you
>>>>>
>>>>> -Hamlin
>>>>>
>>>>> ------------------------------------------------------------------------ 
>>>>>
>>>>>
>>>>> diff -r 0dec8c41170c test/jdk/java/rmi/testlibrary/TestLibrary.java
>>>>> --- a/test/jdk/java/rmi/testlibrary/TestLibrary.java    Wed Jan 17 
>>>>> 20:07:50 2018 -0800
>>>>> +++ b/test/jdk/java/rmi/testlibrary/TestLibrary.java    Thu Jan 18 
>>>>> 14:54:50 2018 +0800
>>>>> @@ -188,8 +188,12 @@
>>>>>      public static long computeDeadline(long timestamp, long 
>>>>> timeout) {
>>>>>          final long MAX_TIMEOUT_MS = 3_600_000L;
>>>>>
>>>>> -        if (timeout < 0L || timeout > MAX_TIMEOUT_MS) {
>>>>> +        if (timeout < 0L) {
>>>>>              throw new IllegalArgumentException("timeout " + 
>>>>> timeout + "ms out of range");
>>>>> +        } else if (timeout > MAX_TIMEOUT_MS) {
>>>>> +            System.out.format("timeout value(%d) exceeds 
>>>>> MAX_TIMEOUT_MS(%d), "
>>>>> +                    + "use MAX_TIMEOUT_MS instead!%n", timeout, 
>>>>> MAX_TIMEOUT_MS);
>>>>> +            timeout = MAX_TIMEOUT_MS;
>>>>>          }
>>>>>
>>>>>          return timestamp + (long)(timeout * getTimeoutFactor());
>>>>>
>>>>
>>>
>