Skip to content

Conversation

@rylev
Copy link
Contributor

@rylev rylev commented Feb 8, 2021

This PR makes several large changes to how error propogation is done in the cosmos crate:

  • Make CosmosError an enum with all errors that are possible to get from using a cosmos API. Some of these will be simple wrappers around generic errors from other crates (e.g., serde_json::Error and azure_core::HeaderError)
  • Use thiserror for declaring errors. Previously, cosmos used failure which has been deprecated in favor of thiserror.

@rylev rylev requested review from MindFlavor, ctaggart and thomastaylor312 and removed request for MindFlavor February 8, 2021 17:18
Copy link
Contributor

@ctaggart ctaggart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good. I will have to attempt updating the services to use thiserror.

Copy link

@thomastaylor312 thomastaylor312 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tenor-255342325

One small blocking change and a few optional nits!

@MindFlavor
Copy link
Contributor

I love it 👍 .

I am curious on how you plan to use CosmosError to capture request specific errors. Do you plan to have different error enums each with a CosmosError variant along with the response specific ones?

@rylev
Copy link
Contributor Author

rylev commented Feb 9, 2021

@MindFlavor which request specific errors do you mean? I believe most requests only error for fairly generic reasons (e.g., unexpected HTTP response status code, bad json, etc.)

@MindFlavor
Copy link
Contributor

MindFlavor commented Feb 9, 2021

Sorry I have been cryptic 👼 ...

I meant, do you mean to expand the CosmosError to capture the meaning of the returned errors? Right now we just return UnexpectedHTTPStatusCode but your work gives us the chance to enhance the user experience by mapping the results with this table: https://docs.microsoft.com/en-us/rest/api/cosmos-db/http-status-codes-for-cosmosdb.

For example, instead of returning "UnexpectedHTTPStatusCode: we expected 200 but we've got 429 back" we could return a variant like CosmosError::TooManyRequests(expected: 200). Also we can mark this type of error as transient.

What do you think?

@rylev
Copy link
Contributor Author

rylev commented Mar 19, 2021

Closing this for now. I'll reopen with a new PR soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants