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. |
Quinn-With-Two-Ns
left a comment
Quinn-With-Two-Ns
left a comment
There was a problem hiding this comment.
Approving, assuming we address the cancel tests and any open SDK team discussions
| * @param endpoint Nexus endpoint name registered on the Temporal Service | ||
| * @param serviceName Nexus service name on that endpoint | ||
| */ | ||
| UntypedNexusServiceClient newUntypedNexusServiceClient(String endpoint, String serviceName); |
There was a problem hiding this comment.
There is no matching method for typed NexusServiceClient. Is it intentional?
| * @return the operation result | ||
| * @throws NexusOperationException if the operation failed, timed out, or was cancelled | ||
| */ | ||
| <U, R> R execute(BiFunction<T, U, R> operation, U input, StartNexusOperationOptions options); |
There was a problem hiding this comment.
Use SDK's Functions.Func2 instead of BiFunction and Func1 instead of Function - applies to whole file.
| @Override | ||
| public UntypedNexusOperationHandle getHandle(String operationId) { | ||
| return getHandle(operationId, null); | ||
| } |
There was a problem hiding this comment.
There is no matching overload for typed handle. Either add one or remove this one (I'd prefer removing this one to make lack of run ID explicit).
| final class CountNexusOperationExecutionsOutput { | ||
| private final long count; | ||
| private final List<AggregationGroup> groups; |
There was a problem hiding this comment.
CountNexusOperationExecutionsOutput could contain NexusOperationExecutionCount directly to simplify architecture.
| * | ||
| * @param input operation ID, optional run ID, and optional human-readable cancellation reason | ||
| */ | ||
| void requestCancelNexusOperationExecution(RequestCancelNexusOperationExecutionInput input); |
There was a problem hiding this comment.
There should be an empty RequestCancelNexusOperationExecutionOutput class that this methods returns, to account for future extensions. Same for terminate and delete.
| @Experimental | ||
| PollActivityExecutionResponse pollActivity(PollActivityExecutionRequest request); |
There was a problem hiding this comment.
Why was pollActivity and listActivities removed?
| ListNexusOperationExecutionsRequest.Builder request = | ||
| ListNexusOperationExecutionsRequest.newBuilder() | ||
| .setNamespace(clientOptions.getNamespace()) | ||
| .setPageSize(LIST_PAGE_SIZE); |
There was a problem hiding this comment.
Do not set explicit page size, instead rely on server default.
| if (!response.hasResult()) { | ||
| return new GetNexusOperationResultOutput<>(null); | ||
| } |
There was a problem hiding this comment.
I'm not sure this is correct behavior? For Temporal activities, a null result is encoded as actual null encoded payload in the result field, and both result and failure missing would be a malformed message. Is Nexus different in this regard?
| public boolean supportsStandaloneNexusOperations() { | ||
| return useExternalService; | ||
| } |
There was a problem hiding this comment.
I feel this unnecessarily duplicates isUseExternalService. Either remove this method, or add some kind of version/capabilities check to ensure the external service does support Nexus and SANO.
4 participants
What was changed
Why?
Checklist
Closes
How was this tested: