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

[Thomas Stüfe]

Hi all,

see here the corrected version:
http://cr.openjdk.java.net/~stuefe/webrevs/8080925/webrev.01/webrev/

I cut out all testing code, the change now is really minimal.

Kind Regards, Thomas


On Thu, Jun 25, 2015 at 2:29 PM, Thomas Stüfe <thomas.stuefe at gmail.com>
wrote:

> Hi all,
>
> thats fine, I will remove the test and repost the change. Thanks for the
> reviews!
>
> Thomas
>
> On Thu, Jun 25, 2015 at 2:09 PM, Christian Tornqvist <
> christian.tornqvist at oracle.com> wrote:
>
>> 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
>> >>>>>>>>
>> >>>>>>>
>> >>>
>>
>>
>>
>