-
Notifications
You must be signed in to change notification settings - Fork 257
Revamp python Continue-as-New documentation #3549
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
Conversation
[Continue-As-New](/workflow-execution/continue-as-new) lets a Workflow Execution close successfully and create a new Workflow Execution. You can think of it as a checkpoint when your Workflow gets too long or approaches certain scaling limits. | ||
|
||
The new Workflow Execution keeps the same Workflow Id but gets a new Run Id and a fresh Event History. | ||
It also receives your Workflow's usual parameters. |
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.
How about a docs-prose-friendly version of the following?
You pass in the input to the new Workflow Run when you Continue-As-New. In practice what this means is that you will design your Workflow parameters so that you can pass in the "current state" when you Continue-As-New. You may want that parameter to default to
None
so that you don't have to pass it when starting the Workflow normally.
Also I think a code sample showing a workflow definition that has been designed to work with Continue-As-New would be great here.
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.
Good suggestion. I did as you suggested, albeit mostly in the next section.
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.
One more suggestion: here, where you say this
lets a Workflow Execution close successfully and create a new Workflow Execution
I found it helpful to consult https://docs.temporal.io/workflow-execution#workflow-execution-chain where "close" and "Workflow Execution" are defined, and it's documented that indeed continue-as-new is one of the ways to close, and what a "Run ID" is.
So maybe a link from this point to there?
|
||
The new Workflow Execution keeps the same Workflow Id but gets a new Run Id and a fresh Event History. | ||
It also receives your Workflow's usual parameters. | ||
Note that Continue-As-New works as an atomic operation. |
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.
I think this needs to be spelled out for users. You're saying either it closes and starts successfully, or doesn't close at all, right? Impossible to close and not start.
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.
On second thought, I think this is probably answering a question nobody has; I'm guessing they'd implicitly assume it was atomic. Removing it unless there's pushback.
|
||
::: | ||
Check this helper each time you consider using Continue-as-New. |
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.
Check this helper each time you consider using Continue-as-New. | |
Check this helper in your test each time you consider using Continue-as-New. |
9658dc5
to
3361c7e
Compare
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.
One more suggestion but LGTM!
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.
On the whole, this looks good. Concise, with the right level of detail, and I like the tone. I made a few suggestions, but nothing major.
|
||
- When the Event History grows too large | ||
- When the Workflow might get too many Updates or Signals | ||
|
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.
If possible, it would be good to elaborate on either of those conditions. is_continue_as_new_suggested()
is a good tool, but readers may want to know what constitutes "too large" or "too many Updates or Signals."
These links might be useful:
https://docs.temporal.io/workflow-execution/event#time-constraints
https://docs.temporal.io/sending-messages#sending-signals
Co-authored-by: Brian MacDonald <[email protected]>
Co-authored-by: Brian MacDonald <[email protected]>
9ba7c94
to
b545fc2
Compare
|
||
### Considerations for Workflows with Message Handlers {#with-message-handlers} | ||
|
||
> 3361c7e8 (Dan feedback) |
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.
fixing
ecf773b
to
9ba7c94
Compare
@@ -65,12 +65,17 @@ An append-only log of [Events](#event) for your application. | |||
- Event History is durably persisted by the Temporal service, enabling seamless recovery of your application state from crashes or failures. | |||
- It also serves as an audit log for debugging. | |||
|
|||
**Event History limits** | |||
### Event History limits {#event-history-limits} |
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.
Note: this section is redundant with some cloud docs, https://docs.temporal.io/cloud/limits#per-workflow-execution-signal-limit , and also with https://docs.temporal.io/workflow-execution/event?_gl=1*9x2uv3*_gcl_au*MjU3MjQ3MTU2LjE3NDAxOTIxMjE.*_ga*MTE5NzU2NzkuMTcxOTQ1MzI2OA..*_ga_R90Q9SJD3D*czE3NDY1NjM0MDEkbzQ2JGcxJHQxNzQ2NTY2MTM2JGowJGwwJGgw#time-constraints
I don't want to link to the former because it's cloud specific.
Don't want to link to the latter because it's not canonical. It should go away or be cut down and just link back to this section.
Overall, this content needs some consolidation, but that's too much to take on in this PR.
Added myself as a reviewer. Going to take a look today and can likely merge this today as well. |
What does this PR do?
is_continue_as_new_suggested
all_handlers_finished
Test Plan
Click all the links
Notes to reviewers