Skip to content
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.

Sign up for GitHub

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

Jump to bottom

EDU-3715: Python update-with-start docs #3250

Merged
merged 14 commits into from
Dec 23, 2024
Merged

EDU-3715: Python update-with-start docs #3250

merged 14 commits into from
Dec 23, 2024

Conversation

dandavison
Copy link
Contributor

@dandavison dandavison commented Dec 19, 2024 •
edited
Loading

  • Should not be merged until Python and Typescript are released
  • Contains some small tweaks to encyclopedia and Go/Java docs
  • Document PHP availability

TODO:

  • check all links after api-docs become available on release

@dandavison dandavison requested a review from a team as a code owner December 19, 2024 18:00
mjameswh
mjameswh reviewed Dec 19, 2024
View reviewed changes
docs/develop/python/message-passing.mdx Outdated

Use [`start_update_with_start_workflow`](https://python.temporal.io/temporalio.client.Client.html#start_update_with_start_workflow) to start an Update and wait for a [`WorkflowUpdateHandle`](https://python.temporal.io/temporalio.client.WorkflowUpdateHandle.html), and use `await update_handle.result()` to retrieve the result from the Update.

Alternatively, use [`execute_update_with_start_workflow`](https://python.temporal.io/temporalio.client.Client.html#start_update_with_start_workflow) to start an Update and wait for the result in one go.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should promote the execute... approach a bit more. It is not simply an "alternative": it is generally preferable if they know that they will need the update result, as it may save one round trip.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right, we should emphasize the execute version since we are emphasizing the use of UwS for low-latency updates. Switched.

mjameswh
mjameswh approved these changes Dec 19, 2024
View reviewed changes
Copy link
Contributor

@mjameswh mjameswh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one minor comment, but I'm ok with what's there already.

brianmacdonald-temporal
brianmacdonald-temporal reviewed Dec 19, 2024
View reviewed changes
Copy link
Contributor

@brianmacdonald-temporal brianmacdonald-temporal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. Holding on merge pending SDK publication.

stephanos
stephanos reviewed Dec 19, 2024
View reviewed changes
docs/develop/python/message-passing.mdx Outdated

In [Public Preview](/evaluate/development-production-features/release-stages#public-preview) in Temporal Cloud.

Minimum Temporal Server version [Temporal Server version 1.26](https://github.com/temporalio/temporal/releases/tag/v1.26.0)
Copy link
Contributor

@stephanos stephanos Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had fixed this yesterday; the tag is wrong: d54486d

(same for the other link in TS)

Copy link
Contributor

@stephanos stephanos Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it seems I'm still using the wrong one in Java and Go.

Could you update that here while you're at it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, done

@brianmacdonald-temporal
Copy link
Contributor

DO NOT MERGE. Changes are approved, but on hold until SDK release.

@stephanos stephanos mentioned this pull request Dec 19, 2024
Closed
@stephanos
Copy link
Contributor

@brianmacdonald-temporal I've cherry-picked the non-blocked changes over here #3257 so it can be merged right now

@fairlydurable fairlydurable added the blocked-on-release These changes are waiting for official release okay label Dec 23, 2024
@fairlydurable fairlydurable changed the title Python and Typescript update-with-start docs EDU-3721: [Do Not Merge] Python and Typescript update-with-start docs Dec 23, 2024
@fairlydurable fairlydurable added cross-team This issue or PR was submitted from within Temporal tracked-internally labels Dec 23, 2024
@dandavison dandavison mentioned this pull request Dec 23, 2024
Merged
@brianmacdonald-temporal brianmacdonald-temporal changed the title EDU-3721: [Do Not Merge] Python and Typescript update-with-start docs EDU-3715: [Do Not Merge] Python and Typescript update-with-start docs Dec 23, 2024
brianmacdonald-temporal
brianmacdonald-temporal approved these changes Dec 23, 2024
View reviewed changes
Copy link
Contributor

@brianmacdonald-temporal brianmacdonald-temporal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good.

@brianmacdonald-temporal brianmacdonald-temporal changed the title EDU-3715: [Do Not Merge] Python and Typescript update-with-start docs EDU-3715: Python update-with-start docs Dec 23, 2024
@brianmacdonald-temporal brianmacdonald-temporal merged commit f0dd85a into main Dec 23, 2024
3 checks passed
@brianmacdonald-temporal brianmacdonald-temporal deleted the python-ts-uws branch December 23, 2024 21:50
@fairlydurable fairlydurable removed the blocked-on-release These changes are waiting for official release okay label Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephanos stephanos stephanos approved these changes

@cretz cretz cretz approved these changes

@mjameswh mjameswh mjameswh approved these changes

@brianmacdonald-temporal brianmacdonald-temporal brianmacdonald-temporal approved these changes

Assignees
No one assigned
Labels
cross-team This issue or PR was submitted from within Temporal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dandavison @brianmacdonald-temporal @stephanos @cretz @mjameswh @fairlydurable