Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

exceptions for load_collection #352

Closed
soxofaan opened this issue Dec 15, 2020 · 5 comments
Closed

exceptions for load_collection #352

soxofaan opened this issue Dec 15, 2020 · 5 comments
Assignees
Milestone

Comments

@soxofaan
Copy link
Member

It might be intentional, but load_collection does not list possible exceptions.

What can go wrong is quite backend dependent, but predefining some well chosen generic errors might improve troubleshooting for end user or backend maintainer. For example:

  • StorageUnavailable
  • DataCorruption
  • RequestTooLarge
@m-mohr
Copy link
Member

m-mohr commented Dec 22, 2020

With these it feels like they are better suited here:
https://openeo.org/documentation/1.0/developers/api/errors.html

But on the other hand, it really is a bit unclear which exceptions to add to the general-purpose list and which to add to the processes. The question is how extensive the list of errors in the processes should be. The three examples sound like very specific and rare exceptions that are not really things a user could circumvent. And I'd think for the specific exceptions for processes I'd only list those that actually come from a user "mis-behavior" (like HTTP 400 errors), not from server errors (like HTTP 500 errors).

So maybe to have a general rule:

  • errors due to invalid user input -> process exception in openeo-processes
  • server errors (and anything not bound to a process) -> errors.json in API

All three examples above for me are server errors (although one could debate that RequestTooLarge could be a user error, but it's likely hard for users to guess what "too large" is). So I'm proposing to move this issue to openeo-api.

@soxofaan
Copy link
Member Author

soxofaan commented Jan 4, 2021

I'm not sure these errors should be considered "general-purpose", as they are quite backend-dependent and specifically tied to data loading. By putting them in the process spec, a backend can easily drop or add errors, while errors.json in API should probably be a bit more standardized and rigid.

I'd think for the specific exceptions for processes I'd only list those that actually come from a user "mis-behavior" (like HTTP 400 errors), not from server errors (like HTTP 500 errors).

I'm not sure. It doesn't seem useful to me to leverage that distinction when deciding where to define an error. There are already plenty of errors in errors.json that are about "user mis-behavior" and not about server errors. And sometimes it's not possible to determine (in advance) whether the user or the server is "wrong" (e.g. if a file path can not be found, is it because the user made a path typo, or is it because of a wrong server-side mount?)

@m-mohr
Copy link
Member

m-mohr commented Jan 4, 2021

I'm not sure these errors should be considered "general-purpose", as they are quite backend-dependent and specifically tied to data loading.

If they are very specific, they should likely not be in the backend independent processes? Also, I think as a rule of thumb errors should usually be referenced in a description so that it's clear when they should be thrown.

By putting them in the process spec, a backend can easily drop or add errors

Back-ends can still add exceptions at any point to their process definition.

I'd think for the specific exceptions for processes I'd only list those that actually come from a user "mis-behavior" (like HTTP 400 errors), not from server errors (like HTTP 500 errors).

I'm not sure. It doesn't seem useful to me to leverage that distinction when deciding where to define an error. There are already plenty of errors in errors.json that are about "user mis-behavior" and not about server errors.

Yes, but we are speaking processes here, not endpoints. errors.json has 400 errors here because it's for endpoints. You can only define them here. For processes, I'd still think my proposal above makes sense.

And sometimes it's not possible to determine (in advance) whether the user or the server is "wrong" (e.g. if a file path can not be found, is it because the user made a path typo, or is it because of a wrong server-side mount?)

I don't get how this supports your other arguments. Why is this relevant here?

My point still is that we can't anticipate all possible server errors and I'm not even sure we need to inform the user specifically. He can't do anything about server errors except contacting the support and wait. So it doesn't help him to know the exact reason, only the support must know it. [What I would do as a provider, is just reporting to the user "Server side error, details/a snapshot have been saved. Please contact support and provide us with the error id." Support can then look up the cause and fix it.]

@soxofaan
Copy link
Member Author

soxofaan commented Jan 4, 2021

I don't get how this supports your other arguments.

I was using this example to illustrate that the distinction between "user fault" (400) and "server fault" (500) is not always very clear. I know you have to put a status code on it, but that might be arbitrary (or biased: e.g. use 500 when in doubt). But in the end I would not use that as argument to put one in errors.json and the other in the process spec.

To me the distinction between errors.json vs openeo-processes is just endpoint-related errors (former) vs process related errors (latter).

I'm not even sure we need to inform the user specifically. He can't do anything about server errors except contacting the support and wait.

Unless the user has more control over the backend than we usually/currently assume: e.g. user is running own backend instance in some cloud-like solution.

But anyway, my initial post was just minor suggestion, a backend can construct error message/info to own liking anyway. Issue importance is not proportional to the current length of this discussion 😸 Feel free to close as "no change required"

@m-mohr
Copy link
Member

m-mohr commented Jan 4, 2021

To me the distinction between errors.json vs openeo-processes is just endpoint-related errors (former) vs process related errors (latter).

I can also live with this distinction. In this case there no common ground for server-side errors in processes, but I think that is fine as they are often pretty specific.

I've taken the chance to clarify in the API a little bit how process exceptions are meant to be defined, see the commit. Thanks.

@m-mohr m-mohr closed this as completed Jan 4, 2021
@m-mohr m-mohr transferred this issue from Open-EO/openeo-processes Jan 4, 2021
@m-mohr m-mohr added this to the 1.1.0 milestone Jan 4, 2021
@m-mohr m-mohr self-assigned this Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants