Skip to content

Commit 8613b18

Browse files
Quinn-With-Two-NsQuinn-With-Two-Ns
authored
Fix nexus error translation (#2539)
Fix nexus error translation
1 parent 5d64818 commit 8613b18

File tree

5 files changed

+64
-21
lines changed

5 files changed

+64
-21
lines changed

‎temporal-sdk/src/main/java/io/temporal/internal/nexus/NexusTaskHandlerImpl.java

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import io.nexusrpc.OperationException;
99
import io.nexusrpc.handler.*;
1010
import io.temporal.api.common.v1.Payload;
11+
import io.temporal.api.enums.v1.NexusHandlerErrorRetryBehavior;
1112
import io.temporal.api.nexus.v1.*;
1213
import io.temporal.client.WorkflowClient;
1314
import io.temporal.client.WorkflowException;
@@ -42,7 +43,6 @@ public class NexusTaskHandlerImpl implements NexusTaskHandler {
4243
private final Map<String, ServiceImplInstance> serviceImplInstances =
4344
Collections.synchronizedMap(new HashMap<>());
4445
private final ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(1);
45-
private final WorkerInterceptor[] interceptors;
4646
private final TemporalInterceptorMiddleware nexusServiceInterceptor;
4747

4848
public NexusTaskHandlerImpl(
@@ -55,7 +55,7 @@ public NexusTaskHandlerImpl(
5555
this.namespace = Objects.requireNonNull(namespace);
5656
this.taskQueue = Objects.requireNonNull(taskQueue);
5757
this.dataConverter = Objects.requireNonNull(dataConverter);
58-
this.interceptors = Objects.requireNonNull(interceptors);
58+
Objects.requireNonNull(interceptors);
5959
this.nexusServiceInterceptor = new TemporalInterceptorMiddleware(interceptors);
6060
}
6161

@@ -131,6 +131,7 @@ public Result handle(NexusTask task, Scope metricsScope) throws TimeoutException
131131
HandlerError.newBuilder()
132132
.setErrorType(e.getErrorType().toString())
133133
.setFailure(exceptionToNexusFailure(e.getCause(), dataConverter))
134+
.setRetryBehavior(mapRetryBehavior(e.getRetryBehavior()))
134135
.build());
135136
} catch (Throwable e) {
136137
return new Result(
@@ -151,6 +152,18 @@ public Result handle(NexusTask task, Scope metricsScope) throws TimeoutException
151152
}
152153
}
153154

155+
private NexusHandlerErrorRetryBehavior mapRetryBehavior(
156+
HandlerException.RetryBehavior retryBehavior) {
157+
switch (retryBehavior) {
158+
case RETRYABLE:
159+
return NexusHandlerErrorRetryBehavior.NEXUS_HANDLER_ERROR_RETRY_BEHAVIOR_RETRYABLE;
160+
case NON_RETRYABLE:
161+
return NexusHandlerErrorRetryBehavior.NEXUS_HANDLER_ERROR_RETRY_BEHAVIOR_NON_RETRYABLE;
162+
default:
163+
return NexusHandlerErrorRetryBehavior.NEXUS_HANDLER_ERROR_RETRY_BEHAVIOR_UNSPECIFIED;
164+
}
165+
}
166+
154167
private void cancelOperation(OperationContext context, OperationCancelDetails details) {
155168
try {
156169
serviceHandler.cancelOperation(context, details);
@@ -193,7 +206,10 @@ private void convertKnownFailures(Throwable e) {
193206
}
194207
if (failure instanceof ApplicationFailure) {
195208
if (((ApplicationFailure) failure).isNonRetryable()) {
196-
throw new HandlerException(HandlerException.ErrorType.BAD_REQUEST, failure);
209+
throw new HandlerException(
210+
HandlerException.ErrorType.INTERNAL,
211+
failure,
212+
HandlerException.RetryBehavior.NON_RETRYABLE);
197213
}
198214
}
199215
if (failure instanceof Error) {

‎temporal-sdk/src/test/java/io/temporal/workflow/nexus/OperationFailMetricTest.java

Lines changed: 38 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,12 @@
2727
import java.util.Map;
2828
import java.util.concurrent.ConcurrentHashMap;
2929
import org.junit.Assert;
30-
import org.junit.Assume;
3130
import org.junit.Rule;
3231
import org.junit.Test;
3332

3433
public class OperationFailMetricTest {
34+
private static Map<String, Integer> invocationCount = new ConcurrentHashMap<>();
35+
3536
private final TestStatsReporter reporter = new TestStatsReporter();
3637

3738
@Rule
@@ -43,7 +44,6 @@ public class OperationFailMetricTest {
4344
new RootScopeBuilder()
4445
.reporter(reporter)
4546
.reportEvery(com.uber.m3.util.Duration.ofMillis(10)))
46-
.setUseExternalService(false)
4747
.build();
4848

4949
private ImmutableMap.Builder<String, String> getBaseTags() {
@@ -81,6 +81,7 @@ public void failOperationMetrics() {
8181

8282
WorkflowFailedException workflowException =
8383
Assert.assertThrows(WorkflowFailedException.class, () -> workflowStub.execute("fail"));
84+
assertNoRetries("fail");
8485
ApplicationFailure applicationFailure =
8586
assertNexusOperationFailure(ApplicationFailure.class, workflowException);
8687
Assert.assertEquals("intentional failure", applicationFailure.getOriginalMessage());
@@ -107,6 +108,7 @@ public void failOperationApplicationErrorMetrics() {
107108

108109
WorkflowFailedException workflowException =
109110
Assert.assertThrows(WorkflowFailedException.class, () -> workflowStub.execute("fail-app"));
111+
assertNoRetries("fail-app");
110112
ApplicationFailure applicationFailure =
111113
assertNexusOperationFailure(ApplicationFailure.class, workflowException);
112114
Assert.assertEquals("intentional failure", applicationFailure.getOriginalMessage());
@@ -135,8 +137,10 @@ public void failHandlerBadRequestMetrics() {
135137
WorkflowFailedException workflowException =
136138
Assert.assertThrows(
137139
WorkflowFailedException.class, () -> workflowStub.execute("handlererror"));
140+
assertNoRetries("handlererror");
138141
HandlerException handlerException =
139142
assertNexusOperationFailure(HandlerException.class, workflowException);
143+
Assert.assertEquals(HandlerException.ErrorType.BAD_REQUEST, handlerException.getErrorType());
140144
Assert.assertTrue(handlerException.getCause() instanceof ApplicationFailure);
141145
ApplicationFailure applicationFailure = (ApplicationFailure) handlerException.getCause();
142146
Assert.assertEquals("handlererror", applicationFailure.getOriginalMessage());
@@ -165,8 +169,10 @@ public void failHandlerAppBadRequestMetrics() {
165169
WorkflowFailedException workflowException =
166170
Assert.assertThrows(
167171
WorkflowFailedException.class, () -> workflowStub.execute("handlererror-app"));
172+
assertNoRetries("handlererror-app");
168173
HandlerException handlerException =
169174
assertNexusOperationFailure(HandlerException.class, workflowException);
175+
Assert.assertEquals(HandlerException.ErrorType.BAD_REQUEST, handlerException.getErrorType());
170176
Assert.assertTrue(handlerException.getCause() instanceof ApplicationFailure);
171177
ApplicationFailure applicationFailure = (ApplicationFailure) handlerException.getCause();
172178
Assert.assertEquals("intentional failure", applicationFailure.getOriginalMessage());
@@ -192,19 +198,24 @@ public void failHandlerAppBadRequestMetrics() {
192198

193199
@Test
194200
public void failHandlerAlreadyStartedMetrics() {
195-
Assume.assumeFalse("skipping", true);
196201
TestWorkflow1 workflowStub =
197202
testWorkflowRule.newWorkflowStubTimeoutOptions(TestWorkflow1.class);
198203
WorkflowFailedException workflowException =
199204
Assert.assertThrows(
200205
WorkflowFailedException.class, () -> workflowStub.execute("already-started"));
201-
ApplicationFailure applicationFailure =
202-
assertNexusOperationFailure(ApplicationFailure.class, workflowException);
206+
assertNoRetries("already-started");
207+
HandlerException handlerException =
208+
assertNexusOperationFailure(HandlerException.class, workflowException);
209+
Assert.assertEquals(HandlerException.ErrorType.BAD_REQUEST, handlerException.getErrorType());
210+
Assert.assertTrue(handlerException.getCause() instanceof ApplicationFailure);
211+
ApplicationFailure applicationFailure = (ApplicationFailure) handlerException.getCause();
203212
Assert.assertEquals(
204213
"io.temporal.client.WorkflowExecutionAlreadyStarted", applicationFailure.getType());
205214

206215
Map<String, String> execFailedTags =
207-
getOperationTags().put(MetricsTag.TASK_FAILURE_TYPE, "operation_failed").buildKeepingLast();
216+
getOperationTags()
217+
.put(MetricsTag.TASK_FAILURE_TYPE, "handler_error_BAD_REQUEST")
218+
.buildKeepingLast();
208219
Eventually.assertEventually(
209220
Duration.ofSeconds(3),
210221
() -> {
@@ -224,6 +235,7 @@ public void failHandlerRetryableApplicationFailureMetrics() {
224235
testWorkflowRule.newWorkflowStubTimeoutOptions(TestWorkflow1.class);
225236
Assert.assertThrows(
226237
WorkflowFailedException.class, () -> workflowStub.execute("retryable-application-failure"));
238+
Assert.assertTrue(invocationCount.get("retryable-application-failure") > 1);
227239

228240
Map<String, String> execFailedTags =
229241
getOperationTags()
@@ -253,12 +265,16 @@ public void failHandlerNonRetryableApplicationFailureMetrics() {
253265
() -> workflowStub.execute("non-retryable-application-failure"));
254266
HandlerException handlerFailure =
255267
assertNexusOperationFailure(HandlerException.class, workflowException);
268+
assertNoRetries("non-retryable-application-failure");
269+
256270
Assert.assertTrue(handlerFailure.getMessage().contains("intentional failure"));
257-
Assert.assertEquals(HandlerException.ErrorType.BAD_REQUEST, handlerFailure.getErrorType());
271+
Assert.assertEquals(HandlerException.ErrorType.INTERNAL, handlerFailure.getErrorType());
272+
Assert.assertEquals(
273+
HandlerException.RetryBehavior.NON_RETRYABLE, handlerFailure.getRetryBehavior());
258274

259275
Map<String, String> execFailedTags =
260276
getOperationTags()
261-
.put(MetricsTag.TASK_FAILURE_TYPE, "handler_error_BAD_REQUEST")
277+
.put(MetricsTag.TASK_FAILURE_TYPE, "handler_error_INTERNAL")
262278
.buildKeepingLast();
263279
Eventually.assertEventually(
264280
Duration.ofSeconds(3),
@@ -298,6 +314,7 @@ public void failHandlerErrorMetrics() {
298314
testWorkflowRule.newWorkflowStubTimeoutOptions(TestWorkflow1.class);
299315
WorkflowFailedException workflowException =
300316
Assert.assertThrows(WorkflowFailedException.class, () -> workflowStub.execute("error"));
317+
Assert.assertTrue(invocationCount.get("error") > 1);
301318

302319
Map<String, String> execFailedTags =
303320
getOperationTags()
@@ -324,6 +341,13 @@ public void handlerErrorNonRetryableMetrics() {
324341
WorkflowFailedException workflowException =
325342
Assert.assertThrows(
326343
WorkflowFailedException.class, () -> workflowStub.execute("handlererror-nonretryable"));
344+
assertNoRetries("handlererror-nonretryable");
345+
HandlerException handlerFailure =
346+
assertNexusOperationFailure(HandlerException.class, workflowException);
347+
Assert.assertTrue(handlerFailure.getMessage().contains("intentional failure"));
348+
Assert.assertEquals(HandlerException.ErrorType.INTERNAL, handlerFailure.getErrorType());
349+
Assert.assertEquals(
350+
HandlerException.RetryBehavior.NON_RETRYABLE, handlerFailure.getRetryBehavior());
327351

328352
Map<String, String> execFailedTags =
329353
getOperationTags()
@@ -342,6 +366,10 @@ public void handlerErrorNonRetryableMetrics() {
342366
});
343367
}
344368

369+
private void assertNoRetries(String testCase) {
370+
Assert.assertEquals(new Integer(1), invocationCount.get(testCase));
371+
}
372+
345373
public static class TestNexus implements TestWorkflow1 {
346374
@Override
347375
public String execute(String operation) {
@@ -360,17 +388,13 @@ public String execute(String operation) {
360388

361389
@ServiceImpl(service = TestNexusServices.TestNexusService1.class)
362390
public class TestNexusServiceImpl {
363-
Map<String, Integer> invocationCount = new ConcurrentHashMap<>();
364-
365391
@OperationImpl
366392
public OperationHandler<String, String> operation() {
367393
// Implemented inline
368394
return OperationHandler.sync(
369395
(ctx, details, operation) -> {
370-
invocationCount.put(
371-
details.getRequestId(),
372-
invocationCount.getOrDefault(details.getRequestId(), 0) + 1);
373-
if (invocationCount.get(details.getRequestId()) > 1) {
396+
invocationCount.put(operation, invocationCount.getOrDefault(operation, 0) + 1);
397+
if (invocationCount.get(operation) > 1) {
374398
throw OperationException.failure(new RuntimeException("exceeded invocation count"));
375399
}
376400
switch (operation) {

‎temporal-sdk/src/test/java/io/temporal/workflow/nexus/OperationFailureConversionTest.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ public void nexusOperationApplicationFailureNonRetryableFailureConversion() {
4040
Assert.assertTrue(nexusFailure.getCause() instanceof HandlerException);
4141
HandlerException handlerException = (HandlerException) nexusFailure.getCause();
4242
Assert.assertTrue(handlerException.getMessage().contains("failed to call operation"));
43-
Assert.assertEquals(HandlerException.ErrorType.BAD_REQUEST, handlerException.getErrorType());
43+
Assert.assertEquals(HandlerException.ErrorType.INTERNAL, handlerException.getErrorType());
4444
}
4545

4646
@Test
@@ -55,7 +55,7 @@ public void nexusOperationApplicationFailureFailureConversion() {
5555
Assert.assertTrue(nexusFailure.getCause() instanceof HandlerException);
5656
HandlerException handlerFailure = (HandlerException) nexusFailure.getCause();
5757
Assert.assertTrue(handlerFailure.getMessage().contains("exceeded invocation count"));
58-
Assert.assertEquals(HandlerException.ErrorType.BAD_REQUEST, handlerFailure.getErrorType());
58+
Assert.assertEquals(HandlerException.ErrorType.INTERNAL, handlerFailure.getErrorType());
5959
}
6060

6161
public static class TestNexus implements TestWorkflow1 {

‎temporal-sdk/src/test/java/io/temporal/workflow/nexus/SyncClientOperationTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ public void syncClientOperationFail() {
9696
Map<String, String> execFailedTags =
9797
ImmutableMap.<String, String>builder()
9898
.putAll(operationTags)
99-
.put(MetricsTag.TASK_FAILURE_TYPE, "handler_error_BAD_REQUEST")
99+
.put(MetricsTag.TASK_FAILURE_TYPE, "handler_error_INTERNAL")
100100
.buildKeepingLast();
101101
reporter.assertCounter(MetricsType.NEXUS_EXEC_FAILED_COUNTER, execFailedTags, 1);
102102
}

‎temporal-test-server/src/main/java/io/temporal/internal/testservice/TestWorkflowService.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1038,7 +1038,10 @@ private static Failure handlerErrorToFailure(HandlerError err) {
10381038
return Failure.newBuilder()
10391039
.setMessage(err.getFailure().getMessage())
10401040
.setNexusHandlerFailureInfo(
1041-
NexusHandlerFailureInfo.newBuilder().setType(err.getErrorType()).build())
1041+
NexusHandlerFailureInfo.newBuilder()
1042+
.setType(err.getErrorType())
1043+
.setRetryBehavior(err.getRetryBehavior())
1044+
.build())
10421045
.setCause(nexusFailureToAPIFailure(err.getFailure(), false))
10431046
.build();
10441047
}

0 commit comments

Comments
 (0)