-
Notifications
You must be signed in to change notification settings - Fork 150
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
Update Java SDK for Temporal Sever v1.26.2 #2357
Update Java SDK for Temporal Sever v1.26.2 #2357
Conversation
@@ -189,7 +189,14 @@ private TemporalFailure failureToExceptionImpl(Failure failure, DataConverter da | |||
} | |||
case FAILUREINFO_NOT_SET: | |||
default: | |||
throw new IllegalArgumentException("Failure info not set"); | |||
// All unknown types are considered to be retryable ApplicationError. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked all other SDKs and the Java SDK was the only SDK that was throwing an exception here instead of converting to an application failure
@@ -58,17 +58,6 @@ public void syncOperationImmediatelyCancelled() { | |||
"operation canceled before it was started", canceledFailure.getOriginalMessage()); | |||
} | |||
|
|||
@Test | |||
public void syncOperationCancelled() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this test since the Server dropped support for cancelling failing sync operations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor non-blocking comments
Assert.assertEquals( | ||
"operation terminated", | ||
((ApplicationFailure) nexusFailure.getCause()).getOriginalMessage()); | ||
// TODO: Test server needs to be fixed to return the correct type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this TODO is to be done separately, can we open an issue and reference it here?
@@ -86,6 +87,17 @@ | |||
import org.slf4j.LoggerFactory; | |||
|
|||
class TestWorkflowMutableStateImpl implements TestWorkflowMutableState { | |||
static final Failure FAILED_UPDATE_ON_WF_COMPLETION = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a test confirming that this at least looks mostly like the real server's version? Unsure if Java has tests against actual server and test server with the same code, but I just want to make sure if/when this changes server side we can catch it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I can do that
There was some changes in Temporal Sever v1.26.2. This PR updates the Java SDK tests and test server to handle the new behaviour