FrancisG-Massey / Capstone2016

4 stars 0 forks source link

Create request failure policy #220

Open FrancisG-Massey opened 7 years ago

FrancisG-Massey commented 7 years ago

When a create request (such as POST /catch) fails, the default behaviour is to keep it in the update queue & try to re-send it again with the next request batch (which occurs once every 30 seconds).

This means if a request keeps failing, the app will keep trying to re-send it once every 30 seconds (while internet is available). This obviously isn't desired, but the alternative of throwing it away entirely could be a problem if the API isn't accepting requests due to a bug (either with the app or the API), as it means data gets lost.

How should we handle a failed request to POST a catch log/trap creation from the app?

NOTE: The app currently distinguishes network issues (where it gets no HTTP response from the server at all) and stops sending any further requests automatically until the user makes a successful manual request (such as pressing the 'refresh' button or clicking 'send catch logs'). It also distinguishes authorisation issues due to session timeouts (or otherwise).

sam-hunt commented 7 years ago

I think this is a reasonably good policy and we could safely stick with it for requests where no HTTP response comes back. You could have the default reconnect attempt period increase with each failed request. But you'd want to put an upper limit on that for sure, and if you've got other priorities, I wouldn't rank this highly at all.

Of the failure responses, ideally only 400s would be worth aborting as they will never succeed without modification or other preconditions being met. If the API was perfect, 500s should always be repeatable and be able to succeed at a later time, but currently, some semantic 400s resolve to 500s due to unparsed SQLExceptions as we discussed on Friday.

FrancisG-Massey commented 7 years ago

Ideally, if conflicts could return their own status code, in those cases the app could throw away the data (as it either means the catch was already logged/trap already created, or something went so badly wrong on the app that the data isn't usable anyway). Though given the time limitations I guess it's easier just to throw away the data if any 4xx error occurs, as currently the app log is getting spammed with a bunch of failed HTTP requests every 30 seconds (and I'd assume the same is true with the API log)

sam-hunt commented 7 years ago

Yeah that is probably the case. Its safe to assume 4xx are discardable. Even once the error parsing on the API gets done (its a huge task since it comes paired with #177, and might not be be on the books given the time remaining), a conflict would be HTTP 409 CONFLICT, which is obviously 4xx too.

FrancisG-Massey commented 7 years ago

Leaving this open for now, as I'd like to distinguish between 5xx and 4xx error codes (which currently doesn't happen as there's no ResponseState available for it). However, this can now be marked as an optional "enhancement"

ZoeUdy commented 7 years ago

"Easy fix done" - possibly more work to be done to provide a nicer solution