ga4gh / task-execution-schemas

Apache License 2.0
80 stars 28 forks source link

Rename ERROR -> EXECUTOR_ERROR #88

Closed buchanae closed 6 years ago

buchanae commented 6 years ago

I think this makes it much clearer that this state only comes from a failed Executor process.

geoffjentry commented 6 years ago

👍

pditommaso commented 6 years ago

Is it really useful for the client to know if it's a task error or system error (I mean, can the client manage/react differently to these errors)? Maybe this information should go in a error cause message (provided there's such field).

buchanae commented 6 years ago

For me, having the different error types helps me as a user understand whether I did something wrong in my task/container, or whether the system failed. In the case of the system failing, I know it's more likely that I need to restart the task, debug the infrastructure, contact the sysadmin, etc.

geoffjentry commented 6 years ago

Yeah we get asked all the time if something is a cromwell error or e.g. pipelines api. You can combine the concepts but having them as independent entities is a huge help

pditommaso commented 6 years ago

For me, having the different error types helps me as a user understand whether I did something wrong in my task/container

Exactly, for this reason I'm suggesting that this information should go in a descriptive error cause, more than a separate api endpoint.

buchanae commented 6 years ago

Exactly, for this reason I'm suggesting that this information should go in a descriptive error cause, more than a separate api endpoint.

I believe that's what this is. It's a different value in a field, and the cause could go in the system_logs field in #80. The different enum value ensures that programs, such as a javascript dashboard, may also understand the difference without requiring special text matching rules.

pditommaso commented 6 years ago

👍

buchanae commented 6 years ago

Bah! This was short-sighted. I missed the case where a task fails because it's trying to upload something that doesn't exist, in which case EXECUTOR_ERROR doesn't make sense: https://github.com/ohsu-comp-bio/funnel/issues/387

Maybe we need an error reason, like @pditommaso said. Maybe it could be an enum.