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!
| @Nonnull DescribeNexusOperationExecutionRequest request, @Nonnull Deadline deadline); | ||
|
|
||
| // ---- Standalone Activity RPCs ---- | ||
| CompletableFuture<DescribeNexusOperationExecutionResponse> describeNexusOperationExecutionAsync( |
There was a problem hiding this comment.
I think we can remove describeNexusOperationExecutionAsync we should only have APIs here that are actually used by the high level clients/handles
There was a problem hiding this comment.
Remove the async one
There was a problem hiding this comment.
This is still here?
|
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)
| import java.util.concurrent.TimeoutException; | ||
| import javax.annotation.Nullable; | ||
|
|
||
| public interface UntypedNexusOperationHandle { |
There was a problem hiding this comment.
We still need Docs for this class and methods
| private final @Nullable String runId; | ||
| private final boolean includeInput; | ||
| private final boolean includeOutcome; | ||
| private final @Nonnull Deadline deadline; |
There was a problem hiding this comment.
Why does this API take a deadline?
| final class DescribeNexusOperationExecutionInput { | ||
| private final String operationId; | ||
| private final @Nullable String runId; | ||
| private final boolean includeInput; |
There was a problem hiding this comment.
We don't need to expose all these options in the interceptor, I think we can just copy how describe activity was implemented https://github.com/temporalio/sdk-java/blob/master/temporal-sdk/src/main/java/io/temporal/common/interceptors/ActivityClientCallsInterceptor.java#L250
|
|
||
| /** Snapshot of a standalone Nexus operation execution returned by describe/poll calls. */ | ||
| @Experimental | ||
| public final class NexusClientOperationExecutionDescription { |
There was a problem hiding this comment.
| public final class NexusClientOperationExecutionDescription { | |
| public final class NexusOperationExecutionDescription { |
I think this can just be?
| this.response = response; | ||
| } | ||
|
|
||
| /** Run ID of the operation described. */ |
There was a problem hiding this comment.
I would expect a lot more fields here like https://github.com/temporalio/sdk-java/blob/master/temporal-sdk/src/main/java/io/temporal/client/ActivityExecutionDescription.java#L28
|
Still a lot of classes missing javadocs comments or missing nots like the parameters, return type or what they throw. A good reference would be https://github.com/temporalio/sdk-java/blob/master/temporal-sdk/src/main/java/io/temporal/client/ActivityHandle.java |
|
|
||
| /** | ||
| * Optional. Unique identifier for this operation within its namespace. If unset, the SDK | ||
| * generates a random UUID. |
There was a problem hiding this comment.
This parameter should be required
| } | ||
|
|
||
| /** 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
| public class NexusOperationHandleImpl<R> implements NexusOperationHandle<R> { | ||
|
|
||
| /** Default deadline applied to per-handle non-poll RPCs (e.g. {@code describe}). */ | ||
| private static final long DEFAULT_DEADLINE_SECONDS = 30; |
There was a problem hiding this comment.
We should not be defining or needing deadlines here
| * Constructed untyped by {@link NexusClient#getHandle(String)} and bound to a result type via | ||
| * {@link NexusOperationHandle#fromUntyped}. | ||
| */ | ||
| public class NexusOperationHandleImpl<R> implements NexusOperationHandle<R> { |
There was a problem hiding this comment.
The implementation here is a bit flipped from the stand alone activity implementation . The SAA code the typed handle just uses the untyped handle internally. There isn't a strong reason I can see for them to be so different so I would standardize on one to keep the maintenance burden low. Since the SAA one is already approved I would standardize on that.
| * Operation} annotation when set, otherwise the Java method name. | ||
| */ | ||
| public static String nexusOperationName(Method method) { | ||
| Operation op = method.getAnnotation(Operation.class); |
There was a problem hiding this comment.
You don't need to parse the annotations manually you should use the proper Nexus SDK methods to parse the annotation ServiceDefinition.fromClass
2 participants
What was changed
Why?
Checklist
Closes
How was this tested: