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

feat(error): init error handling philosophy #111

Merged
merged 38 commits into from
Nov 17, 2023
Merged

feat(error): init error handling philosophy #111

merged 38 commits into from
Nov 17, 2023

Conversation

guergabo
Copy link
Contributor

@guergabo guergabo commented Nov 14, 2023

Changes

Fixes #95
Fixes #96
Fixes #98
Fixes #99

Context

Error Duality

Expected Errors

  1. application level errors (tolerable, expected, non-retryable) -> ResponseStatus && validation layer using json tags -- protobuf

  2. platform level errors (tolerable, expected, retryable) -> errors.New() then convert to ResponseStatus at api layer

Unexpected Errors

  1. catastrophic (intolerable, unexpected) -> panic()

internal/api/errors.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Attention: 155 lines in your changes are missing coverage. Please review.

Comparison is base (2c924b7) 67.16% compared to head (42a68a4) 68.61%.
Report is 4 commits behind head on main.

Files Patch % Lines
internal/app/subsystems/api/http/promise.go 23.07% 40 Missing ⚠️
internal/app/subsystems/api/service/service.go 45.45% 25 Missing and 5 partials ⚠️
internal/app/subsystems/api/grpc/api/promise.pb.go 6.89% 27 Missing ⚠️
internal/app/subsystems/api/grpc/grpc.go 73.68% 9 Missing and 1 partial ⚠️
internal/app/coroutines/cancelPromise.go 36.36% 7 Missing ⚠️
internal/kernel/t_api/status.go 91.42% 6 Missing ⚠️
internal/app/coroutines/createPromise.go 37.50% 5 Missing ⚠️
internal/app/coroutines/rejectPromise.go 44.44% 5 Missing ⚠️
internal/app/coroutines/resolvePromise.go 44.44% 5 Missing ⚠️
internal/app/coroutines/readPromise.go 33.33% 4 Missing ⚠️
... and 10 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #111      +/-   ##
==========================================
+ Coverage   67.16%   68.61%   +1.45%     
==========================================
  Files          57       61       +4     
  Lines        5944     6163     +219     
==========================================
+ Hits         3992     4229     +237     
+ Misses       1667     1654      -13     
+ Partials      285      280       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@guergabo guergabo marked this pull request as ready for review November 16, 2023 04:18
@guergabo
Copy link
Contributor Author

guergabo commented Nov 16, 2023

@vaibhawvipul let's talk tmrrw about this pr. this changes a bit of the expected api behavior. for example introducing 409 status code and returning error messages for unsuccessful requests instead of nil or promises. the sdk will need to adjust a bit. additionally, the client deciding which errors are retryable becomes trivial once this is merged.

For grpc, we return proper grpc error codes now and status in response is only to capture dedup logic (a.k.a, did this request change something in the database or not).

internal/kernel/t_api/status.go Outdated Show resolved Hide resolved
internal/app/coroutines/cancelPromise.go Outdated Show resolved Hide resolved
internal/api/errors.go Outdated Show resolved Hide resolved
internal/api/errors.go Outdated Show resolved Hide resolved
@guergabo guergabo self-assigned this Nov 16, 2023
@guergabo guergabo requested a review from dfarr November 17, 2023 16:54
@guergabo guergabo requested a review from dfarr November 17, 2023 17:01
internal/aio/aio.go Outdated Show resolved Hide resolved
internal/api/api.go Outdated Show resolved Hide resolved
@guergabo guergabo merged commit a5efda2 into resonatehq:main Nov 17, 2023
5 checks passed
@guergabo guergabo deleted the errors branch November 17, 2023 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants