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.

Sign up for GitHub

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

Jump to bottom

New failure propagation #100

Merged
merged 8 commits into from
Jul 24, 2023
Merged

New failure propagation #100

merged 8 commits into from
Jul 24, 2023

Conversation

slinkydeveloper
Copy link
Contributor

@slinkydeveloper slinkydeveloper commented Jul 17, 2023 •
edited
Loading

Fix #94. This PR implements the new failure propagation. The logic is the following:

  • Every failure that is a StatusRuntimeFailure with status code != UNKNOWN is a terminal failure
  • in all the other cases is a non terminal failure. This includes ProtocolException, user thrown exceptions, etc.

@slinkydeveloper
Implement ErrorMessage header
1cf11d0
@slinkydeveloper
Refactor ProtocolException to hold code numbers outside the gRPC Stat…
fca9f81 
…us.Code range
@slinkydeveloper
Write out the ErrorMessage when there is a failure cause.
18372a5
@slinkydeveloper
* Now when closing the state machine, we invoke syscalls.fail(cause) …
ab8e0bc 
…on every failure with Code UNKNOWN. In other words, a failure is non-terminal when code is UNKNOWN, otherwise is terminal.

* Now GrpcServerCallListenerAdaptor won't erase the cause anymore when getting an exception from the user code, because we need it for syscalls.fail(cause)
@slinkydeveloper
Make sure we don't write non-terminal failures for side effects
2b6bc32
@slinkydeveloper
Tests
5888b9e
@github-actions
Copy link
Contributor

github-actions bot commented Jul 17, 2023 •
edited
Loading

Unit Test Results

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

Results for commit 2bb3045. ± Comparison against base commit 0093ff2.

This pull request removes 2 and adds 14 tests. Note that renamed tests count towards both. 
dev.restate.sdk.core.impl.UserFailuresTest ‑ 1: [BUFFERED_SINGLE_THREAD] FailingGreeter
dev.restate.sdk.core.impl.UserFailuresTest ‑ 2: [UNBUFFERED_MULTI_THREAD] FailingGreeter

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 ‑ 13: [BUFFERED_SINGLE_THREAD] SideEffectThrowStatusRuntimeException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 14: [UNBUFFERED_MULTI_THREAD] SideEffectThrowStatusRuntimeException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 1: [BUFFERED_SINGLE_THREAD] ThrowIllegalStateException
dev.restate.sdk.core.impl.UserFailuresTest ‑ 2: [UNBUFFERED_MULTI_THREAD] ThrowIllegalStateException
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
…

♻️ This comment has been updated with latest results.

@slinkydeveloper
Copy link
Contributor Author

slinkydeveloper commented Jul 17, 2023 •
edited
Loading

Initially I wanted to use as rule for "terminal/non terminal" the following:

  • Every failure that is a StatusRuntimeFailure is a terminal failure
  • in all the other cases is a non terminal failure

Essentially keeping away the condition of Code != UNKNOWN. Unfortunately this is not easy to achieve, due to the fact that the code generated by the gRPC compiler uses Status all around, so it's not easy to distinguish between terminal/non terminal Status. Even managing somehow to signal the correct terminal/non terminal type from ExceptionCatchingInvocationInputSubscriber to RestateServerCall (one idea being the usage of Metadata or the Context) , one could still use responseObserver.onError and there I have no control of the translation from exception to our failure types.

tillrohrmann
tillrohrmann approved these changes Jul 23, 2023
View reviewed changes
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.

Thanks for creating this PR @slinkydeveloper. The changes look good to me. Before merging, it would be good to somehow document the behavior for our users (I guess via some JavaDocs, later once we have a Java SDK documentation then also there).

sdk-core-impl/src/main/java/dev/restate/sdk/core/impl/RestateServerCall.java Outdated Show resolved Hide resolved
sdk-core-impl/src/main/java/dev/restate/sdk/core/impl/SyscallsImpl.java Outdated
@@ -202,6 +203,9 @@ public void exitSideEffectBlockWithException(
if (protocolException.isPresent()) {
throw protocolException.get();
}
if (!(toWrite instanceof StatusRuntimeException)) {
throw (RuntimeException) toWrite;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this cast safe? Can't this be any other unchecked exception here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the side effect lambda cannot through non-RuntimeException. In any case, I added a wrapping to avoid any future unsafe usage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we wrapping all unchecked exceptions in a RuntimeException (also Error)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this stage we cannot really do anything else, can't we? it's the caller that perhaps shouldn't catch Error when invoking the side effect call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW check the latest code

sdk-core-impl/src/main/java/dev/restate/sdk/core/impl/SyscallsImpl.java Outdated
@@ -202,6 +203,9 @@ public void exitSideEffectBlockWithException(
if (protocolException.isPresent()) {
throw protocolException.get();
}
if (!(toWrite instanceof StatusRuntimeException)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if it is a StatusRuntimeException and code == UNKNOWN?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it will be treated as non-terminal I guess. Perhaps let me align this with #100 (comment)

@slinkydeveloper slinkydeveloper mentioned this pull request Jul 24, 2023
Merged
tillrohrmann
tillrohrmann approved these changes Jul 24, 2023
View reviewed changes
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.

Thanks for resolving my comments. LGTM. +1 for merging.

@slinkydeveloper slinkydeveloper merged commit e1a6389 into main Jul 24, 2023
3 checks passed
@slinkydeveloper slinkydeveloper deleted the issues/94 branch July 24, 2023 15:27
@slinkydeveloper slinkydeveloper mentioned this pull request Jul 25, 2023
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tillrohrmann tillrohrmann tillrohrmann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adopt ErrorMessage
2 participants
@slinkydeveloper @tillrohrmann