Schedules #279
Schedules #279
Conversation
Fixes temporalio#87 Fixes temporalio#258
|
I am aware of CI breaks both due to my assumptions around list consistency and Windows builds currently breaking repo wide. I am working on those, but don't let it halt review. |
| await assert_no_schedules(client) | ||
|
|
||
| # Just in case it's on the minute boundary, move it off | ||
| now = datetime.utcnow() |
There was a problem hiding this comment.
Didn't we decide this is not needed since the backfill start range is exclusive?
There was a problem hiding this comment.
Harmless in the test to be safe and intentionally test off the boundary
There was a problem hiding this comment.
I'd argue its misleading if a user read this test, but not blocking this PR
|
|
||
|
|
||
| async def assert_no_schedules(client: Client) -> None: | ||
| # Listing appears eventually consistent |
There was a problem hiding this comment.
nit: remove comment now that this is confirmed?
There was a problem hiding this comment.
That makes the comment even more important to explain why I'm repeatedly getting count
temporalio/client.py
Outdated
| """ | ||
|
|
||
| recent_actions: Sequence[ScheduleActionResult] | ||
| """10 most recent actions, oldest first.""" |
There was a problem hiding this comment.
👍 I will remove the counts on the list one. But we need to update https://typescript.temporal.io/api/interfaces/client.ScheduleSummary#info too. cc @mjameswh.
EDIT: Removed specifics about count since API docs don't have them either for list info.
There was a problem hiding this comment.
Yeah the problem is the number returned by list is different then describe.
https://github.com/temporalio/temporal/blob/master/service/worker/scheduler/workflow.go#L148
There was a problem hiding this comment.
I removed the exact count on both update and describe in TS.
I know the spec mention 10 on ScheduleInfo, but given that this was made tweakable on the server, I think that we'd better consider that number as an implementation detail rather than a contract.
| ) | ||
| ) | ||
|
|
||
| def get_schedule_handle(self, id: str) -> ScheduleHandle: |
There was a problem hiding this comment.
Might be useful to mention somewhere that get_schedule_handle doesn't validate that the schedule indeed exists, and thus, that describe and others might fail...
There was a problem hiding this comment.
Meh, not needed for get_workflow_handle or get_async_activity_handle. Since they are sync calls to fetching a handle, I think it's clear.
| ScheduleCalendarSpec._from_proto(c) for c in spec.structured_calendar | ||
| ], | ||
| intervals=[ScheduleIntervalSpec._from_proto(i) for i in spec.interval], | ||
| cron_expressions=spec.cron_string, |
There was a problem hiding this comment.
You should never receive a cron_string from the server. I'd consider removing that line.
There was a problem hiding this comment.
This will then always be an empty sequence of strings which is safe and still reads nice from a symmetry perspective. So which fields are populated remains a server decision.
What was changed
Sorry there is so much here, it's a large API. But most lines are just structure and comments. Use https://github.dev/temporalio/sdk-python/pull/279 w/ side-by-side and for the most of this you can ignore the diff that git is showing except for a few minor actual changes to existing code.
I decided that this fits best in the client package even though it makes that file quite big (that is the Python way quite often). I have also decided that the features repo tests weren't enough for properly testing everything so I have integration tests herein with the expectation to add the feature tests upon release (pretty soon after merged).
Checklist