RFR(xs): 8080925: Make error log write timeout parameter configurable

Thu Jun 25 12:09:00 UTC 2015

Hi Thomas,

I agree with Coleen, I think you shouldn't add the test. It'll likely be flaky in some situations and cause us issues, the change seems safe enough and you've manual verified it.

Thanks,
Christian

-----Original Message-----
From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of Coleen Phillimore
Sent: Thursday, June 25, 2015 7:50 AM
To: hotspot-runtime-dev at openjdk.java.net
Subject: Re: RFR(xs): 8080925: Make error log write timeout parameter configurable

My preference is to not add the test.   You verified it manually and 
it's in an area of code that isn't likely to accidentally change so needs a regression test every day.

I'd be happy to sponsor the revision without the test.
Coleen

On 6/25/15 2:41 AM, Thomas Stüfe wrote:
> Hi Christian, David,
>
> yes, I admit you are right. We run similar tests since a long time at 
> SAP, and even with a lot of leeway we run into situations where the VM 
> does not finish in time. Very rarely (maybe once a month) but still. 
> Especially on older hardware, like AS/400. In almost all situations 
> these were false positives, only once this was a legitimate error.
>
> My suggestions would be
>
> 1) Either I could just not test. The change is arguably easy enough to 
> omit a specific test.
> 2) Or I could add a change (which would be a good idea anyway) to, 
> before I abort, print out a last line into the hs-err file: "-- 
> Timeout During Error Reporting after xxx seconds --". In the test, 
> instead of measuring time, I would scan for this trailing line in the 
> hs-err file. That would have the additional advantage that we later know why the hs-err file is torn.
>
> I also could do (1) now, and (2) in a separate patch - I have 
> vacations coming up in a week and would like to finish this beforehand.
>
> Thomas
>
>
> On Thu, Jun 25, 2015 at 7:34 AM, David Holmes 
> <david.holmes at oracle.com>
> wrote:
>
>> Hi Thomas,
>>
>> On 25/06/2015 2:01 AM, Christian Tornqvist wrote:
>>
>>> Hi Thomas,
>>>
>>> I'm concerned with the time measurement. We run a lot of our tests 
>>> concurrently on a wide spread of hardware, I wouldn't be surprised 
>>> if it sometimes took more than 4s to get the JVM to a state where 
>>> the ErrorHandlerTests are invoked.
>>>
>> I share Christian's concern, particularly as you start measuring 
>> before even exec'ing the new JVM:
>>
>>    70     long t1 = System.currentTimeMillis();
>>    71
>>    72     OutputAnalyzer output_detail = new OutputAnalyzer(pb.start());
>>
>> There's really no reliable way to validate a timeout using an 
>> external
>> observer: you either have so much leeway that you won't spot an 
>> unexpected delay; or too little such that some platforms/devices 
>> don't make it. The framework timeout will at least detect if the 
>> underlying VM execution fails to terminate at all.
>>
>> Sorry I don't have any better suggestions for testing this.
>>
>> David
>> -----
>>
>>
>>
>>   Thanks,
>>> Christian
>>>
>>> -----Original Message-----
>>> From: hotspot-runtime-dev [mailto:
>>> hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of Coleen 
>>> Phillimore
>>> Sent: Wednesday, June 24, 2015 11:45 AM
>>> To: hotspot-runtime-dev at openjdk.java.net
>>> Subject: Re: RFR(xs): 8080925: Make error log write timeout 
>>> parameter configurable
>>>
>>>
>>> Hi Thomas,
>>>
>>> This change looks okay, I guess.  I've been avoiding tests that 
>>> crash the JVM because I don't know if this will mess up test execution in any
>>> way.   Also I guess I don't see any other way to add the test without a
>>> command line parameter, like TestSafeFetchInErrorHandler , but I feel
>>> like we're swamped in command line parameters!   But you just got rid of
>>> one that was broken, so I guess this is fine.
>>>
>>> I can sponsor your change if someone from our SQE group okays the test.
>>>
>>> Thanks,
>>> Coleen
>>>
>>> On 6/24/15 11:30 AM, Thomas Stüfe wrote:
>>>
>>>> Hi,
>>>>
>>>> This change is in review limbo since a month...
>>>>
>>>> Could I have a second reviewer, please, and a sponsor?
>>>>
>>>> Or, if the change is not wanted, a reason why?
>>>>
>>>> Thanks a lot,
>>>>
>>>> Thomas
>>>>
>>>>
>>>> On Mon, Jun 15, 2015 at 2:58 PM, Thomas Stüfe 
>>>> <thomas.stuefe at gmail.com>
>>>> wrote:
>>>>
>>>>   Hi all,
>>>>> may I have a second review for this tiny change? I also need a sponsor.
>>>>>
>>>>> Thanks!
>>>>>
>>>>> On Fri, May 22, 2015 at 4:21 PM, Thomas Stüfe 
>>>>> <thomas.stuefe at gmail.com>
>>>>> wrote:
>>>>>
>>>>>   Hi Staffan,
>>>>>> thanks!
>>>>>>
>>>>>> ...Thomas
>>>>>>
>>>>>> On Fri, May 22, 2015 at 1:55 PM, Staffan Larsen < 
>>>>>> staffan.larsen at oracle.com> wrote:
>>>>>>
>>>>>>   Looks good to me.
>>>>>>> A small nit in globals.hpp:925: "an timeout” -> “a timeout”. No 
>>>>>>> need for a new webrev if you fix this.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> /Staffan
>>>>>>>
>>>>>>>   On 22 maj 2015, at 13:33, Thomas Stüfe 
>>>>>>> <thomas.stuefe at gmail.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> please review this small change:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8080925/webrev.00/we
>>>>>>>> bre
>>>>>>>> v/
>>>>>>>>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8080925
>>>>>>>>
>>>>>>>> This is a small addition which makes the timeout to write error 
>>>>>>>> log
>>>>>>>>
>>>>>>> files
>>>>>>>
>>>>>>>> configurable.
>>>>>>>>
>>>>>>>> Thanks & Kind Regards,
>>>>>>>>
>>>>>>>> Thomas
>>>>>>>>
>>>>>>>
>>>