Add support for more timeouts to Nexus operations#1276
Add support for more timeouts to Nexus operations#1276tconley1428 merged 6 commits intotemporalio:mainfrom
Conversation
|
|
|
|
Quinn-With-Two-Ns
force-pushed
the
NEXUS-21
branch
from
0b541a2 to
e1c9abe
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2554cee0e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| schedule_to_start_timeout: timedelta | None | ||
| start_to_close_timeout: timedelta | None |
There was a problem hiding this comment.
StartNexusOperationInput is part of the public interceptor API, but these newly added fields are required constructor args, which is a backwards-incompatible break for existing interceptors that instantiate this dataclass (or pass positional args) using the prior signature. After this change, older code will either raise TypeError for missing args or mis-bind positional values, so adding defaults (e.g., None) is needed to preserve compatibility while introducing the new timeouts.
Useful? React with 👍 / 👎.
| nexus_task_executor=concurrent.futures.ThreadPoolExecutor(), | ||
| ): | ||
| await create_nexus_endpoint(task_queue, client) | ||
| try: |
There was a problem hiding this comment.
I prefer pytest.raises, but no big deal.
| @workflow.defn | ||
| class StartToCloseTimeoutTestCallerWorkflow: | ||
| @workflow.init | ||
| def __init__( |
There was a problem hiding this comment.
Any particular reason to create the nexus client as a member? Not that it matters too much in the test, but if we're viewing all code as AI consumed, is this a good pattern to suggest?
There was a problem hiding this comment.
That's actually what all the tests in this file do so i just copied them for consistency.
Quinn-With-Two-Ns
force-pushed
the
NEXUS-21
branch
from
992fdfb to
c76138c
Compare
Add schedule-to-start and start-to-close for Nexus operations
Quinn-With-Two-Ns
force-pushed
the
NEXUS-21
branch
from
c76138c to
c7b0ff0
Compare
|
@tconley1428 Good to merge? |
3 participants
Add schedule-to-start and start-to-close for Nexus operations
NOTE, before this can be merged the API still needs to be tagged, core changes need to be merged and tests need to be added
Note
Medium Risk
Expands a public API surface and changes how Nexus operations are scheduled by adding new timeout fields, which could affect operation execution semantics if mis-mapped. Covered by new error-path tests, but relies on server/core support for the new proto fields.
Overview
Adds support for two additional Nexus operation timeouts by threading
schedule_to_start_timeoutandstart_to_close_timeoutthrough the workflow Nexus client API, worker runtime interceptors, and the scheduledschedule_nexus_operationcommand.Updates the public
NexusClient.start_operation/execute_operationsignatures and_Runtime.workflow_start_nexus_operationto accept these new parameters, and adds new integration tests assertingTimeoutErrorsurfaces withTimeoutType.SCHEDULE_TO_STARTandTimeoutType.START_TO_CLOSEwhen those limits are exceeded.Written by Cursor Bugbot for commit a780707. This will update automatically on new commits. Configure here.