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

Implement test server support for sync Nexus operation commands #2176

Merged
merged 8 commits into from
Aug 15, 2024

Conversation

pdoerner
Copy link
Contributor

@pdoerner pdoerner commented Aug 9, 2024

What was changed

Added test server support for Nexus operation commands, including sync completion, timeouts, failures, and retries. Added most of the stubs for implementing asynchronous operations in followup PRs.

The test server will only support worker targets, not external, so there is no added logic for making HTTP requests.
The implementation largely follows the same design as ActivityTasks.

@pdoerner pdoerner changed the title [WIP] Implement test server support for sync Nexus operations Implement test server support for sync Nexus operation commands Aug 13, 2024
@pdoerner pdoerner marked this pull request as ready for review August 13, 2024 14:36
@pdoerner pdoerner requested a review from a team as a code owner August 13, 2024 14:36
import org.junit.Rule;
import org.junit.Test;

public class NexusWorkflowTest {
Copy link
Contributor Author

@pdoerner pdoerner Aug 13, 2024

Choose a reason for hiding this comment

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

I know these tests have a lot of copy-pasted code; didn't want to spend a ton of time on test design in this PR. Planning to add more coverage and refactor common code when writing async operation tests.

out.writeInt(attempt);
return ByteString.copyFrom(bout.toByteArray());
} catch (IOException e) {
throw Status.INTERNAL.withCause(e).withDescription(e.getMessage()).asRuntimeException();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the real server also throw an INTERNAL error if the task token is bad?

Copy link
Contributor

Choose a reason for hiding this comment

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

If this was the core Java SDK I would probably advocate for doing the try catch to a GRPC specific exception outside of toBytes to separate the transport details from the implementation. For the test server it is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good catch, it throws INVALID_ARGUMENT; I'll update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I think serialization error is INTERNAL and deserialization is INVALID_ARGUMENT so that's what I've changed it to.

Copy link
Contributor

@Quinn-With-Two-Ns Quinn-With-Two-Ns left a comment

Choose a reason for hiding this comment

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

LGTM, nothing stands out as obviously wrong, but I'll admit I don't know how the nexus task state machine on the server looks and if it matches the test server. Relying on the tests to ensure we have very close behaviour here.

@Quinn-With-Two-Ns
Copy link
Contributor

This Test test failure here looks problematic, seems like an upsert search attribute test was treated as a nexus operation

    event_id: 4
    event_time { seconds: 1723665245 nanos: 673000000 }
    event_type: EVENT_TYPE_WORKFLOW_TASK_FAILED
    workflow_task_failed_event_attributes { 
      scheduled_event_id: 2
      started_event_id: 3
      cause: WORKFLOW_TASK_FAILED_CAUSE_BAD_SCHEDULE_NEXUS_OPERATION_ATTRIBUTES
      failure { message: "BadScheduleNexusOperationAttributes: Could not find Nexus endpoint with name: " source: "JavaSDK" stack_trace: "io.temporal.internal.testservice.CommandVerifier.verifyCommand(CommandVerifier.java:69)\nio.temporal.internal.testservice.TestWorkflowMutableStateImpl.lambda$completeWorkflowTask$4(TestWorkflowMutableStateImpl.java:439)\nio.temporal.internal.testservice.TestWorkflowMutableStateImpl.update(TestWorkflowMutableStateImpl.java:285)\nio.temporal.internal.testservice.TestWorkflowMutableStateImpl.completeWorkflowTaskUpdate(TestWorkflowMutableStateImpl.java:263)\nio.temporal.internal.testservice.TestWorkflowMutableStateImpl.completeWorkflowTask(TestWorkflowMutableStateImpl.java:387)\nio.temporal.internal.testservice.TestWorkflowService.respondWorkflowTaskCompleted(TestWorkflowService.java:501)\nio.temporal.api.workflowservice.v1.WorkflowServiceGrpc$MethodHandlers.invoke(WorkflowServiceGrpc.java:6057)\nio.grpc.stub.ServerCalls$UnaryServerCallHandler$UnaryServerCallListener.onHalfClose(ServerCalls.java:182)\nio.grpc.internal.ServerCallImpl$ServerStreamListenerImpl.halfClosed(ServerCallImpl.java:355)\nio.grpc.internal.ServerImpl$JumpToApplicationThreadServerStreamListener$1HalfClosed.runInContext(ServerImpl.java:867)\nio.grpc.internal.ContextRunnable.run(ContextRunnable.java:37)\nio.grpc.internal.SerializingExecutor.run(SerializingExecutor.java:133)\njava.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)\njava.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)\njava.base/java.lang.Thread.run(Thread.java:1589)\n" server_failure_info { non_retryable: true } }
      identity: "25083@fv-az695-304"
    }

@pdoerner pdoerner merged commit abc5323 into master Aug 15, 2024
11 checks passed
@pdoerner pdoerner deleted the test-server-nexus-commands branch August 15, 2024 16:56
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.

2 participants