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

[Thomas Stüfe]

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
> >>>>>>>>
> >>>>>>>
> >>>
>
>
>