865758 - Improve error handling of Mongodb error cases

Status: RESOLVED FIXED

(Webmaker Graveyard :: Login, defect)

Description

[Kieran Sedgwick (:sedge)]

At the moment, the server continues to run in the case of a Mongodb connection error.  Instead, it should kill the process and complain loudly.

Comment 1

[Kieran Sedgwick (:sedge)]

Attachment: Pull request for this patch

https://github.com/mozilla/login.webmaker.org/pull/28

Comment 2

[Ross Bruniges [:boozeniges]]

I think for something important this feels a bit to simple for what we're looking for.

This will only alert in the console that things have broken yeah?

Comment 3

[Ross Bruniges [:boozeniges]]

Comment on attachment 743098 [details] [review]
Pull request for this patch

I did the review wrong - but comment above: ^^

Comment 4

[Kieran Sedgwick (:sedge)]

Right now it's just a console message.  This patch would cause the server to exit in the case that a mongo database can't be found.  This would only happen if it can't find a database immediately after the server starts, so I think it's a good fix.

Comment 5

[Ross Bruniges [:boozeniges]]

Have a look with the makeAPI folk to see what they're doing to handle things.

If they're cool with is so am I

Comment 6

[Simon C. Wex]

This is actually up to JP. He's doing some testing with autoscale groups to see what the time in and out is like. If he doesn't want the process to exit, we should write a little library that handles a /healthcheck endpoint and the various dependent connections a process needs.

Comment 7

[Kieran Sedgwick (:sedge)]

Attachment: Pull request for this patch

https://github.com/mozilla/login.webmaker.org/pull/30

Comment 9

[Ross Bruniges [:boozeniges]]

Comment on attachment 743259 [details] [review]
Pull request for this patch

A couple of nits regarding coding style - but the logic behind things looks good.

r+ assuming the nits and code style is cleaned up

Comment 10

[Kieran Sedgwick (:sedge)]

Comment on attachment 743259 [details] [review]
Pull request for this patch

Further work to be done..

Comment 11

[Kieran Sedgwick (:sedge)]

Comment on attachment 743259 [details] [review]
Pull request for this patch

The changes are now stable.

Comment 12

[David Humphrey (:humph)]

Comment on attachment 743259 [details] [review]
Pull request for this patch

A few comments in the pull request.  One big issue is that I think you want to change all the `400` result codes to `404` for the case where a user ID isn't found.  A `400` means "bad syntax" where `404` means "resource not found."

Comment 13

[Kieran Sedgwick (:sedge)]

Comment on attachment 743259 [details] [review]
Pull request for this patch

Changed the 400 error codes

Comment 14

[David Humphrey (:humph)]

Comment on attachment 743259 [details] [review]
Pull request for this patch

14:02 < humph> Let's change the error message
14:02 < sedge> okay
14:02 < humph> not that it failed
14:02 < humph> but that there was no connection
14:03 < sedge> "No connection found"
14:03 < sedge> ok
14:03 < daleee> mjschranz: thank you, did not know about fillmurray
14:03 < humph> and this is probably OK
14:03 < sedge> for now anyway?
14:03 < humph> file a new bug on integrating with the /healthcheck endpoint
14:03 < sedge> okay
14:03 < sedge> on it

r+ with those things done.

Comment 15

[Kieran Sedgwick (:sedge)]

https://github.com/ksedge/login.webmaker.org/commit/8c9613ee56d0644630d60617cf7c161207e00fef

Comment 16

[Ross Bruniges [:boozeniges]]

Comment on attachment 743259 [details] [review]
Pull request for this patch

Can we close the review on this ticket?