Conversation
| * type binding to an {@link UntypedNexusClientHandle} (returned by {@link | ||
| * NexusClient#getHandle(String)}) by calling one of the {@link #fromUntyped} factories. | ||
| */ | ||
| public interface NexusClientHandle<R> extends UntypedNexusClientHandle { |
There was a problem hiding this comment.
| public interface NexusClientHandle<R> extends UntypedNexusClientHandle { | |
| public interface NexusOperationHandle<R> extends UntypedNexusOperationHandle { |
Please keep consistent naming with other Handles like https://github.com/temporalio/sdk-java/blob/master/temporal-sdk/src/main/java/io/temporal/client/ActivityHandle.java
There was a problem hiding this comment.
The nexus doc had "NexusOperationHandle". I didn't use that name as there was already a NexusOperationHandle class and I didn't want to duplicate it. Even if in a different package, that just seemed confusing.
The reason I went to NexusClient in a lot of class names was to avoid naming collisions like that - again, even if in different packages it seemed confusing, and I wanted some form of consistency. So I named this NexusClientHandle to show that it was linked to the other NexusClient classes.
That being said, I do want to be consistent, but might have to check in with you and talk this one out. Maybe we can make these NexusOperationExecutionHandle and UntypedNexusOperationExecutionHandle, but then we lose the link to the other NexusClient classes - though would we use these for all Nexus operations so maybe we don't need such a link?
There was a problem hiding this comment.
Renamed to NexusOperationHandle as per conversation - we feel that the names being in different packages that have different use cases which should never be mixed should be sufficient to avoid confusion, especially as the user will get this returned to them and won't be creating these classes.
Leaving this conversation unresolved to make sure the SDK team sees it and can weigh in!
|
Reviewed most of the public API, didn't get to into the tests or implementation for now since some stuff will likely change. |
…nt, typed and untyped service clients)
| } | ||
|
|
||
| /** Nexus protocol headers forwarded to the handler. */ | ||
| public Builder setNexusHeader(@Nullable Map<String, String> nexusHeader) { |
There was a problem hiding this comment.
Generally in Temporal headers are only exposed in interceptors
There was a problem hiding this comment.
Removed. I do see a propagatedHeader method in ActivityClientImpl that uses a ContextPropagator class - I added something similar for Nexus. Headers in Nexus are just strings but they are Payload in Activity. I see SyncWorkflowContext just using the value as a string so I did the same.
Resolve this thread if it looks OK now?
| PollNexusOperationExecutionOutput pollNexusOperationExecution( | ||
| PollNexusOperationExecutionInput input); | ||
|
|
||
| CompletableFuture<PollNexusOperationExecutionOutput> pollNexusOperationExecutionAsync( |
There was a problem hiding this comment.
Looks like for SAA we exposed GetResult not Poll, we should sync the with SDK team here https://github.com/temporalio/sdk-java/blob/master/temporal-sdk/src/main/java/io/temporal/common/interceptors/ActivityClientCallsInterceptor.java#L51
| } | ||
|
|
||
| /** Sync poll loop bounded by an absolute nanos deadline. */ | ||
| private PollNexusOperationExecutionOutput pollSyncUntilCompletedOrDeadline(long deadlineNanos) |
There was a problem hiding this comment.
Please look at
ideally we would reuse the same core loop for all these pollings if possibleThere was a problem hiding this comment.
I took a shot at this but ... there are a lot of different types, and it made this pretty ugly.
| if (System.nanoTime() >= deadlineNanos) { | ||
| TimeoutException timeout = | ||
| new TimeoutException("getResult timed out before the operation completed"); | ||
| timeout.initCause(e); |
There was a problem hiding this comment.
We do you need to construct the TimeoutException and set the cause separately?
|
Where are the Nexus Operation Failure exceptions when the operation fails ? I would expect to see at least |
| } | ||
|
|
||
| @Test | ||
| public void listNexusOperationExecutions() { |
There was a problem hiding this comment.
This test and the count are really not testing a lot, we should at least make sure some operations are running so we can now they are parsing the response correctly
| NexusOperationExecutionCount output = client.countNexusOperationExecutions(null); | ||
|
|
||
| Assert.assertNotNull(output); | ||
| Assert.assertTrue(output.getCount() >= 0); |
There was a problem hiding this comment.
This test doesn't trigger any operations so I can't see how this would pass unless other tests happen to run before it?
| UntypedNexusOperationHandle handle = svcClient.start("operation", opts, inputValue); | ||
| String operationId = handle.getNexusOperationId(); | ||
|
|
||
| // Sync handler: wait for the input to land in the test side-channel; that's how we |
There was a problem hiding this comment.
I think we can simplify this test a bit and just wait for the operations result? Testing that we can get the result of the operation covers the whole end to end flow.
| .setWorkflowTypes(PlaceholderWorkflowImpl.class) | ||
| .setNexusServiceImplementation(new TestNexusServiceImpl()) | ||
| // Default is 10s; standalone Nexus dispatch + worker poll can take longer. | ||
| .setTestTimeoutSeconds(120) |
There was a problem hiding this comment.
Thought we said we would remove this
| } | ||
|
|
||
| /** Holder for state used to drive a single test against one started operation. */ | ||
| private static final class StartedOperation { |
There was a problem hiding this comment.
Do we need a whole class here? Can we just return the UntypedNexusOperationHandle, the client is the same for all the operations
| } | ||
|
|
||
| @Test | ||
| public void cancelSucceedsForStartedOperation() { |
There was a problem hiding this comment.
We should be able to assert more here, we can test the operation was actually canceled by gettings its' result and checking the exception
| } | ||
|
|
||
| @Test | ||
| public void terminateSucceedsForStartedOperation() { |
There was a problem hiding this comment.
| try { | ||
| handle.getResult(String.class); | ||
| Assert.fail("expected getResult to throw because the operation handler failed"); | ||
| } catch (RuntimeException e) { |
There was a problem hiding this comment.
Should be a NexusOperationException
| handle.getResult(String.class); | ||
| Assert.fail("expected getResult to throw because the operation handler failed"); | ||
| } catch (RuntimeException e) { | ||
| // The DataConverter wraps the proto Failure into a Java exception. Either the message |
There was a problem hiding this comment.
We should assert the full exception chain here
| AtomicReference<StartNexusOperationExecutionInput> captured = new AtomicReference<>(); | ||
| RuntimeException sentinel = new RuntimeException("captured-by-test"); | ||
|
|
||
| NexusClientInterceptor recordingFactory = |
There was a problem hiding this comment.
Some of these tests are trying to use the interceptor to make an assertion about what the SDK is doing, but that is not really the assertion we want to test. Checking the interceptor just tells us the SDK forwarded some parameter through the interceptor, it doesn't tell use if the SDK actually sent that parameter to the server properly.
I would refactor these to remove the interceptor and check the server, using describe, to make sure the parameter was properly delivered.
| } | ||
|
|
||
| @ServiceImpl(service = TestNexusServices.TestNexusService1.class) | ||
| public static class TestNexusServiceImpl { |
There was a problem hiding this comment.
I would also add a tests for an async nexus operation, so a Nexus operation started by a workflow.
|
After you address this feedback I would recommend looping in the SDK team to start reviewing as well |
2 participants
What was changed
Why?
Checklist
Closes
How was this tested: