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

Simpler error strategy #102

Merged
merged 1 commit into from
Aug 14, 2023
Merged

Simpler error strategy #102

merged 1 commit into from
Aug 14, 2023

Conversation

slinkydeveloper
Copy link
Contributor

@slinkydeveloper slinkydeveloper commented Jul 25, 2023

Fix #101

This implements a (IMO) simpler error strategy, less error prone:

  • Every thrown failure that is a StatusRuntimeException or a StatusException, or every failure passed to StreamObserver#onError is a terminal failure
  • In all the other cases is a non terminal failure

It adds a bit of "asymmetry" wrt the error types, due to StreamObserver#onError, but i believe this is fine, because the user intentionally calls StreamObserver#onError (as opposed to runtime exceptions that might just be thrown by 3rd party libraries)

@github-actions
Copy link
Contributor

Unit Test Results

144 tests  ±0   144 ✔️ ±0   9s ⏱️ -1s
  14 suites ±0       0 💤 ±0 
  14 files   ±0       0 ±0 

Results for commit 7e25551. ± Comparison against base commit e1a6389.

This pull request removes 10 and adds 10 tests. Note that renamed tests count towards both.
dev.restate.sdk.core.impl.UserFailuresTest ‑ 10: [UNBUFFERED_MULTI_THREAD] ThrowStatusRuntimeException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 11: [BUFFERED_SINGLE_THREAD] ResponseObserverOnErrorStatusRuntimeException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 12: [UNBUFFERED_MULTI_THREAD] ResponseObserverOnErrorStatusRuntimeException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 3: [BUFFERED_SINGLE_THREAD] ThrowUnknownStatusRuntimeException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 4: [UNBUFFERED_MULTI_THREAD] ThrowUnknownStatusRuntimeException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 5: [BUFFERED_SINGLE_THREAD] ResponseObserverOnErrorIllegalStateException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 6: [UNBUFFERED_MULTI_THREAD] ResponseObserverOnErrorIllegalStateException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 7: [BUFFERED_SINGLE_THREAD] SideEffectThrowIllegalStateException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 8: [UNBUFFERED_MULTI_THREAD] SideEffectThrowIllegalStateException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 9: [BUFFERED_SINGLE_THREAD] ThrowStatusRuntimeException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 10: [UNBUFFERED_MULTI_THREAD] ResponseObserverOnErrorStatusRuntimeException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 11: [BUFFERED_SINGLE_THREAD] ResponseObserverOnErrorIllegalStateException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 12: [UNBUFFERED_MULTI_THREAD] ResponseObserverOnErrorIllegalStateException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 3: [BUFFERED_SINGLE_THREAD] SideEffectThrowIllegalStateException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 4: [UNBUFFERED_MULTI_THREAD] SideEffectThrowIllegalStateException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 5: [BUFFERED_SINGLE_THREAD] ThrowStatusRuntimeException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 6: [UNBUFFERED_MULTI_THREAD] ThrowStatusRuntimeException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 7: [BUFFERED_SINGLE_THREAD] ThrowUnknownStatusRuntimeException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 8: [UNBUFFERED_MULTI_THREAD] ThrowUnknownStatusRuntimeException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 9: [BUFFERED_SINGLE_THREAD] ResponseObserverOnErrorStatusRuntimeException

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

I think the model is simpler now with every StatusException, StatusRuntimeException and responseObserver.onError() call being a terminal exception.

@slinkydeveloper slinkydeveloper merged commit d9ebad4 into main Aug 14, 2023
3 checks passed
@slinkydeveloper slinkydeveloper deleted the issues/94 branch August 14, 2023 08:17
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.

Figure out whether it's doable to simplify the new failure handling strategy
2 participants