Elfocrash / Cosmonaut

🌐 A supercharged Azure CosmosDB .NET SDK with ORM support
https://cosmonaut.readthedocs.io
MIT License
342 stars 44 forks source link

Possibility to define when to throw exceptions #79

Closed Mortana89 closed 5 years ago

Mortana89 commented 5 years ago

Hi,

We're using the Mediatr library together with the command/query pipeline within our system. All queries (read) that are being executed, are going through this pipeline, and are implemented using a custom-built base QueryHandler. This QueryHandler is using Polly to do an exponential backoff when there's a high load (TooManyRequests) on the database.

From what I understood when reading the documentation, the docs say that Cosmonaut will throw an exception when the requestrate is too large:

Response Handling Cosmonaut follows a different approach when it comes to error handling. The CosmosDB SDK is throwing exceptions for almost every type of error. Cosmonaut follows a different approach. In Cosmonaut methods that return CosmosResponse or CosmosMultipleResponse won't throw exceptions for the following errors: ResourceNotFound, PreconditionFailed and Conflict. They will instead return a CosmosResponse with the IsSuccess flag to false, the CosmosOperationStatus enum explaining what the error was and the Exception object containing the exceptions that caused the request to fail.

But when going through the code, it looks as if you are, next to the aforementioned 3 statuses, you also catch the TooManyRequests exception. This yields some problems in our queryhandler, as Polly would only act on the exception that is being thrown, and I'd prefer not going down the route of checking the result of a query in every handler manually, which potentially can be forgotten, to throw an exception when appropriate.

Is there either a possibility to configure which of the operation statuses should be thrown as an exception, and which can be handled by the CosmosResponse?

Thanks!

BTW, we completely moved away from EF Core. Not only is our application easily 4x faster in request handling, development is also much more speed up, thanks for the framework!

Elfocrash commented 5 years ago

First and foremost thanks for your kind comments.

Something that Cosmonaut definitely won't do is throw any exception other than the TooManyRequests one. The main reason why is because I removed it first in Cosmonaut and the the Cosmos DB team also removed it on the v3 SDK so I can't add something back that I will have to remove when I move this project to the v3 SDK anyway.

Now, about the TooManyRequests thing, as far as I remember Cosmonaut will actually throw this exception if you turn Infinite retrying off. Did you try doing that?

Mortana89 commented 5 years ago

Aha, I am not setting this parameter explicitly, I assume the default is retry infinitely? I’m happy if it’s going to retry on it’s own, saves me some plumbing work. Just dug into your code to find this parameter and indeed, it’s defaulted to retry indefinitely.

I’ll play around with this to see if I can trigger our own retry policy, as we are trying to capture some metrics/logs during such retry as well.

Thanks! Van: Nick Chapsas notifications@github.com Verzonden: woensdag 3 april 2019 14:16 Aan: Elfocrash/Cosmonaut Cosmonaut@noreply.github.com CC: Yoni Nijs yoni.nijs@zerofriction.co; Author author@noreply.github.com Onderwerp: Re: [Elfocrash/Cosmonaut] Possibility to define when to throw exceptions (#79)

First and foremost thanks for your kind comments.

Something that Cosmonaut definitely won't do is throw any exception other than the TooManyRequests one. The main reason why is because I removed it first in Cosmonaut and the the Cosmos DB team also removed it on the v3 SDK so I can't add something back that I will have to remove when I move this project to the v3 SDK anyway.

Now, about the TooManyRequests thing, as far as I remember Cosmonaut will actually throw this exception if you turn Infinite retrying off. Did you try doing that?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/Elfocrash/Cosmonaut/issues/79#issuecomment-479463858, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ArmKUeHnZ180PyvNBcLqPV1BBW4lvmsoks5vdJtsgaJpZM4caSSj.

Elfocrash commented 5 years ago

Retries should be logged as well. They just retry silently. I set the default behavior to be infinite retry for 429s cause I can't see the scenario where you wouldn't wanna retry due to a transient error.

I've never been like :

Mortana89 commented 5 years ago

Agree! That does explain why a little performance test didn’t break anything, while I was expecting things to fall over when the db was under heavy load (as I was investigating how to prevent this, but it looks like I was trying to prevent something which didn’t need preventing as it was already taken care of😊).

Van: Nick Chapsas notifications@github.com Verzonden: woensdag 3 april 2019 14:25 Aan: Elfocrash/Cosmonaut Cosmonaut@noreply.github.com CC: Yoni Nijs yoni.nijs@zerofriction.co; Author author@noreply.github.com Onderwerp: Re: [Elfocrash/Cosmonaut] Possibility to define when to throw exceptions (#79)

Retries should be logged as well. They just retry silently. I set the default behavior to be infinite retry for 429s cause I can't see the scenario where you wouldn't wanna retry due to a transient error.

I've never been like :

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/Elfocrash/Cosmonaut/issues/79#issuecomment-479466729, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ArmKUS3xvzHiyQTGNUeh_0WpSt5ZvZf4ks5vdJ2VgaJpZM4caSSj.

Elfocrash commented 5 years ago

Ok sounds good. I will leave it with you to figure out if the existing behavior is satisfying. If not feel free to leave a comment. If it is feel free to close this.