codefortulsa / courtbot-engine

5 stars 6 forks source link

Handle partial failures in getCaseParties/Events. #11

Closed pipakin closed 7 years ago

pipakin commented 7 years ago

We should decide how to handle partial failures (one data service fails to retrieve but others succeed). We should, at minimum:

  1. Log all errors to log4js
  2. Continue with the partial data we DO have.

It might be nice to advise the user somehow that we can't retrieve data correctly? A status page would be nice to. Maybe collect failures and show some sort of health indicator?

chriswsh commented 7 years ago

offers suggestion

Would this work?

dataSource.js (e.g. courtbot-engine-oscn-data)

  1. Contains the emitter.on(retrieve-parties) event handler, taking casenumber and result as arguments. (result is an object passed by reference)
  2. Within the event handler, wraps calls to the relevant APIs in Promises     2a. Each Promise either...         ...resolves with the relevant data (array of parties)         ...rejects with { 'case': casenumber, 'API': apiReference, 'timestamp': new Date(), 'error': errorObject }
  3. Pushes the Promises to result.promises[];

events.js --> getCaseParties()

  1. Emits the retrieve-parties event, passing casenumber and results as arguments.
  2. Parses the return values of the newly updated results.promises[]
  3. Logs the info about the Promise rejections.
  4. Emits the retrieve-parties-error event, passing an array of rejections info: [errorObj1, errorObj2, ...]
  5. Returns an array of parties found: [party1, party2, ...]

This way, events.js logs the errors, but then passes them on. This might keep events.js conceptually cleaner: purely an event emitter that collates useful information and logs problems, but doesn't gather or process the information itself.

I like the idea of emitting an event to handle the errors so that the information is split into two streams, and then each stream can be handled separately: one function for the errors, and one for the passed data. Cleaner logic, perhaps?

It would also avoid having to refactor any code that works with the way the module's currently written. Hooray for backwards compatibility! The drawback would be if a calling function wants access to both the parties and the errors in the return value. Maybe another argument with a default value?


casenumber: string which holds the casenumber to look up errorMode: 2-bit integer which determined how rejected Promises are handled     The 1s bit determines if getCaseParties() emits the error information in an event.     The 2s bit determines if getCaseParties() returns the error information with parties found.

function getCaseParties(casenumber, errorMode = 1) {}

pipakin commented 7 years ago

Sounds like a plan. It's unlikely I'll get to code on it much tonigh

pipakin commented 7 years ago

t. Feel free to implement and do a pr. :-)

chriswsh commented 7 years ago

Cool. It's on my schedule for this weekend.

chriswsh commented 7 years ago

The updates to this code pass unit tests. Closing issue.