Add SAA TemporalNexusOperation handling#1565
Conversation
Expose customizable Temporal Nexus operation handlers and client support. Add activity link conversion, operation token handling, and related tests.
VegetarianOrc
changed the title
…it in cancel_activity
| case "activity": | ||
| return activity_link_to_nexus_link(temporal_link.activity) | ||
|
|
||
| case "batch_job" | "workflow": |
There was a problem hiding this comment.
Just future proofing - if this comes through with a unknown value, it will be unhandled - should this be case _ and come after the none instead of specifying two values?
Also you might put the case value in the error message, it would be nice to know when debugging what value was actually recieved.
| return nexusrpc.Link(url=url, type=_LinkType.NEXUS_OPERATION.value) | ||
|
|
||
|
|
||
| def activity_link_to_nexus_link( |
There was a problem hiding this comment.
I don't see any failure handling in this - if urlunparse fails for example, is everything logged and handled in that call?
In looking for failure handling I looked for some unit tests on this method and didn't see any (though I did for nexus_link_to_activity_link). Should there be some tests?
| match operation_token.type: | ||
| case OperationTokenType.WORKFLOW: | ||
| options = CancelWorkflowRunOptions( | ||
| if not operation_token.workflow_id: |
There was a problem hiding this comment.
decode is called above, and the decode method already throws a TypeError if it is missing the workflow or activity ID isn't it? (Lines 99 and 116.) So the error check for workflow here and activity below will never throw, but also might mislead users as they would expect a HandlerErrorType but get a TypeError.
| "Invalid activity operation token: missing activity ID", | ||
| type=HandlerErrorType.NOT_FOUND, | ||
| ) | ||
| if not operation_token.run_id: |
There was a problem hiding this comment.
The decode method doesn't check for run_id like it does the other two - should it? It's nice to have all the error checking on one place, but I don't know if that would affect other things. If not, then this should probably throw TypeError just to stay consistent with the other checks.
(trying to be overly clearn - token.py does check run id, but the other two have two checks - runId is missing the second check.)
|
|
||
| query_params = urllib.parse.parse_qs(url.query) | ||
|
|
||
| match query_params.get(LINK_RUN_ID_PARAM_NAME): |
There was a problem hiding this comment.
When this rolls out, is there any chance links previously serialized with runId as a parameter will be hit? Should this be able to deserialize those to avoid a migration issue? Otherwise things might go out of sync and runId would get lost here.
| LINK_RUN_ID_PARAM_NAME: op_link.run_id, | ||
| }, | ||
| ) | ||
| run_id = urllib.parse.quote(op_link.run_id, safe="") |
There was a problem hiding this comment.
Should there be an error check for namespace, operation_id, and run_id to make sure they got values? Or are we certain these will always be present and so no need to check? Or is is just fine and we'll get a bad URL which will give us a reasonable error later?
3 participants
What was changed
Added
start_activitysupport for Temporal Nexus operation handlers.start_workflowpath to share the backing context, token, and handler behavior needed for activity support.Why?
Temporal Nexus operations should be able to start and cancel activities as well as workflows, using a consistent operation token and backing-operation model across both paths.
Checklist
Added/updated coverage in:
tests/nexus/test_temporal_operation.pytests/nexus/test_operation_token.pytests/nexus/test_link_conversion.pytests/nexus/test_nexus_type_errors.pyYes. Docs should cover start_activity support for Temporal Nexus operation handlers, including operation tokens, cancellation behavior, and activity link handling.