-
Notifications
You must be signed in to change notification settings - Fork 193
Add SAA TemporalNexusOperation handling #1565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
7f260fe
6217763
d071fa5
c821739
f792a2b
611c8ec
3f66527
5004be7
61a4fc0
4d6c02e
a6f33d6
8d602d2
f474758
2b51197
7e108c7
b182df4
3ab946d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,11 @@ | |
| logger = logging.getLogger(__name__) | ||
|
|
||
| _NEXUS_OPERATION_LINK_URL_PATH_REGEX = re.compile( | ||
| r"^/namespaces/(?P<namespace>[^/]+)/nexus-operations/(?P<operation_id>[^/]+)$" | ||
| r"^/namespaces/(?P<namespace>[^/]+)/nexus-operations/(?P<operation_id>[^/]+)/(?P<run_id>[^/]+)/details$" | ||
| ) | ||
|
|
||
| _ACTIVITY_LINK_URL_PATH_REGEX = re.compile( | ||
| r"^/namespaces/(?P<namespace>[^/]+)/activities/(?P<activity_id>[^/]+)/(?P<run_id>[^/]+)/details$" | ||
| ) | ||
|
|
||
| _WORFKLOW_LINK_URL_PATH_REGEX = re.compile( | ||
|
|
@@ -31,13 +35,13 @@ | |
| class _LinkType(str, Enum): | ||
| WORKFLOW = temporalio.api.common.v1.Link.WorkflowEvent.DESCRIPTOR.full_name | ||
| NEXUS_OPERATION = temporalio.api.common.v1.Link.NexusOperation.DESCRIPTOR.full_name | ||
| ACTIVITY = temporalio.api.common.v1.Link.Activity.DESCRIPTOR.full_name | ||
|
|
||
|
|
||
| LINK_EVENT_ID_PARAM_NAME = "eventID" | ||
| LINK_EVENT_TYPE_PARAM_NAME = "eventType" | ||
| LINK_REQUEST_ID_PARAM_NAME = "requestID" | ||
| LINK_REFERENCE_TYPE_PARAM_NAME = "referenceType" | ||
| LINK_RUN_ID_PARAM_NAME = "runID" | ||
|
|
||
| EVENT_REFERENCE_TYPE = "EventReference" | ||
| REQUEST_ID_REFERENCE_TYPE = "RequestIdReference" | ||
|
|
@@ -84,6 +88,9 @@ def nexus_link_to_temporal_link( | |
| case _LinkType.NEXUS_OPERATION: | ||
| return nexus_link_to_nexus_operation_link(nexus_link) | ||
|
|
||
| case _LinkType.ACTIVITY: | ||
| return nexus_link_to_activity_link(nexus_link) | ||
|
|
||
|
|
||
| def temporal_link_to_nexus_link( | ||
| temporal_link: temporalio.api.common.v1.Link, | ||
|
|
@@ -92,16 +99,20 @@ def temporal_link_to_nexus_link( | |
|
|
||
| Returns None when the Temporal link variant is missing. | ||
| """ | ||
| match temporal_link.WhichOneof("variant"): | ||
| variant = temporal_link.WhichOneof("variant") | ||
| match variant: | ||
| case "workflow_event": | ||
| return workflow_event_to_nexus_link(temporal_link.workflow_event) | ||
|
|
||
| case "nexus_operation": | ||
| return nexus_operation_to_nexus_link(temporal_link.nexus_operation) | ||
|
|
||
| case "activity" | "batch_job" | "workflow": | ||
| case "activity": | ||
| return activity_link_to_nexus_link(temporal_link.activity) | ||
|
|
||
| case "batch_job" | "workflow": | ||
| raise NotImplementedError( | ||
| "only workflow_event and nexus operation links are supported" | ||
| f"only workflow_event, activity and nexus_operation links are supported, got {variant}" | ||
| ) | ||
|
|
||
| case None: | ||
|
|
@@ -151,22 +162,30 @@ def nexus_operation_to_nexus_link( | |
| scheme = "temporal" | ||
| namespace = urllib.parse.quote(op_link.namespace, safe="") | ||
| operation_id = urllib.parse.quote(op_link.operation_id, safe="") | ||
| path = f"/namespaces/{namespace}/nexus-operations/{operation_id}" | ||
|
|
||
| query_params = "" | ||
| if op_link.run_id: | ||
| query_params = urllib.parse.urlencode( | ||
| { | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These fields are always populated by the server when they come via proto so safe to assume they're there. If a bug ever existed where they were not populated, they'd wind up as empty strings. If they wind up as empty the server validation will reject appropriately when they're sent along. |
||
| path = f"/namespaces/{namespace}/nexus-operations/{operation_id}/{run_id}/details" | ||
|
|
||
| # urllib will omit '//' from the url if netloc is empty so we add the scheme manually | ||
| url = f"{scheme}://{urllib.parse.urlunparse(('', '', path, '', query_params, ''))}" | ||
| url = f"{scheme}://{urllib.parse.urlunparse(('', '', path, '', '', ''))}" | ||
|
|
||
| return nexusrpc.Link(url=url, type=_LinkType.NEXUS_OPERATION.value) | ||
|
|
||
|
|
||
| def activity_link_to_nexus_link( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This function is exercised in test_link_conversion_nexus_link_to_activity_link where there are a few conversions between nexusrpc.Link <--> proto links for the activity flavor. |
||
| activity: temporalio.api.common.v1.Link.Activity, | ||
| ) -> nexusrpc.Link: | ||
| """Convert an Activity link into a nexusrpc link.""" | ||
| scheme = "temporal" | ||
| namespace = urllib.parse.quote(activity.namespace, safe="") | ||
| activity_id = urllib.parse.quote(activity.activity_id, safe="") | ||
| run_id = urllib.parse.quote(activity.run_id, safe="") | ||
| path = f"/namespaces/{namespace}/activities/{activity_id}/{run_id}/details" | ||
|
|
||
| url = f"{scheme}://{urllib.parse.urlunparse(('', '', path, '', '', ''))}" | ||
|
|
||
| return nexusrpc.Link(url=url, type=_LinkType.ACTIVITY.value) | ||
|
|
||
|
|
||
| def nexus_link_to_workflow_event_link( | ||
| link: nexusrpc.Link, | ||
| ) -> temporalio.api.common.v1.Link | None: | ||
|
|
@@ -230,28 +249,36 @@ def nexus_link_to_nexus_operation_link( | |
| ) | ||
| return None | ||
|
|
||
| query_params = urllib.parse.parse_qs(url.query) | ||
|
|
||
| match query_params.get(LINK_RUN_ID_PARAM_NAME): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fortunately there are no versions of the server that accept nexus-operation links but use the runId as a parameter. |
||
| case [run_id_param]: | ||
| run_id = run_id_param | ||
| case [] | None: | ||
| run_id = "" | ||
| case _: | ||
| logger.warning( | ||
| f"Invalid Nexus link: {nexus_link}. Expected {LINK_RUN_ID_PARAM_NAME} to have at most 1 value" | ||
| ) | ||
| return None | ||
|
|
||
| groups = match.groupdict() | ||
| nexus_op_link = temporalio.api.common.v1.Link.NexusOperation( | ||
| namespace=urllib.parse.unquote(groups["namespace"]), | ||
| operation_id=urllib.parse.unquote(groups["operation_id"]), | ||
| run_id=run_id, | ||
| run_id=urllib.parse.unquote(groups["run_id"]), | ||
| ) | ||
| return temporalio.api.common.v1.Link(nexus_operation=nexus_op_link) | ||
|
|
||
|
|
||
| def nexus_link_to_activity_link( | ||
| nexus_link: nexusrpc.Link, | ||
| ) -> temporalio.api.common.v1.Link | None: | ||
| """Convert a nexus link into a Temporal Activity link.""" | ||
| url = urllib.parse.urlparse(nexus_link.url) | ||
| match = _ACTIVITY_LINK_URL_PATH_REGEX.match(url.path) | ||
| if not match: | ||
| logger.warning( | ||
| f"Invalid Nexus link: {nexus_link}. Expected path to match {_ACTIVITY_LINK_URL_PATH_REGEX.pattern}" | ||
| ) | ||
| return None | ||
|
|
||
| groups = match.groupdict() | ||
| activity_link = temporalio.api.common.v1.Link.Activity( | ||
| namespace=urllib.parse.unquote(groups["namespace"]), | ||
| activity_id=urllib.parse.unquote(groups["activity_id"]), | ||
| run_id=urllib.parse.unquote(groups["run_id"]), | ||
| ) | ||
| return temporalio.api.common.v1.Link(activity=activity_link) | ||
|
|
||
|
|
||
| def _event_reference_to_query_params( | ||
| event_ref: temporalio.api.common.v1.Link.WorkflowEvent.EventReference, | ||
| ) -> str: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern matches against the generated proto files and is future proof in that our linting (and CI) will fail if a new unhandled variant is added.
Will add the case value to the error message for an unsupported type though!