The default bug view has changed. See this FAQ.

Make mercurial closure hooks fail closed once proven reliable



Developer Services
5 years ago
3 years ago


(Reporter: emorley, Assigned: emorley)





(1 attachment, 1 obsolete attachment)

(Chris AtLee [:catlee] from bug 754132 comment #4)
> > If we turn treestatus into a production service, we should consider making
> > this fail closed. treestatus seems simple enough that we could make that
> > work.
> Let's deploy with fail open first and see how it handles things.

Once bug 754132 has been resolved and we're happy with, we should switch to failing closed rather than open - especially given that people can always override the hook using CLOSED TREE anyway, were there to be issues with treestatus.

This will also alleviate the "we want to keep ancient trees closed, but don't want them cluttering up the treestatus UI" problem of bug 758882 comment 0.
Depends on: 759170
Summary: Make mercurial closure hooks fail closed (not open), once proven reliable → Make mercurial closure hooks fail closed once proven reliable
Whiteboard: [sheriff-want]
We should do this.

People managed to land on aurora today due to treestatus HTTP 500 errors :-(
Assignee: nobody →
Keywords: sheriffing-P1
Whiteboard: [sheriff-want]
Created attachment 692978 [details] [diff] [review]
Patch v1
Attachment #692978 - Flags: review?(ted)
Comment on attachment 692978 [details] [diff] [review]
Patch v1

Review of attachment 692978 [details] [diff] [review]:

::: mozhghooks/
@@ +52,5 @@
>      except (ValueError, IOError), (err):
>          # fail open. no sense making hg unavailable
>          # if treestatus is down
>          print "Error: %s" % err, url
> +        return 1

Please make the error message more verbose while you're here, so that people have an understanding of why their push is failing. Also, this print statement is written oddly. It's actually doing print ("Error: %s" % err), url. Maybe tweak that to be less confusing while you're at it?
Attachment #692978 - Flags: review?(ted) → review+
Created attachment 693131 [details] [diff] [review]
Patch v2

I know r+ already, but a few more changes, just wanted to make sure you're ok with making the except a bit more complex.
Attachment #692978 - Attachment is obsolete: true
Attachment #693131 - Flags: review?(ted)
Attachment #693131 - Flags: review?(ted) → review+
Cheers :-)
Depends on: 822648
Last Resolved: 4 years ago
Resolution: --- → FIXED
Product: → Release Engineering

Comment 6

3 years ago
Product: Release Engineering → Developer Services
You need to log in before you can comment on or make changes to this bug.