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.

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

Update core #660

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Update core #660

wants to merge 4 commits into from

Conversation

dandavison
Copy link
Contributor

@dandavison dandavison requested a review from a team as a code owner October 7, 2024 13:47
@cretz
Copy link
Member

cretz commented Oct 7, 2024

May have to upgrade tonic and tonic build (probably just a cargo update to update all deps in the lock would do it, probably don't need toml change)

@cretz
Copy link
Member

cretz commented Oct 8, 2024

@dandavison - I am a little concerned here with temporalio/sdk-core#789 and initialize workflow. It appears that it's more than a name change, it is also an order change. From the PR description:

it might require some changes to langs that are using StartWorkflow as a synonym for "do first iteration of the event loop"? If such changes are needed, they should be the ones described here: #606 (.NET may need an equivalent item) as none of these problems exist if all jobs are all read and applied to state first, before doing any iterating of the event loop.

I think we are saved by the fact that we reorder events ourselves in Python. I can't think of any non-signal/non-update/non-update-random-seed event that ever came before start in Core before that PR was created, so I think we're ok. But may be worth making sure we take a close look and confirm.

@dandavison
Copy link
Contributor Author

I agree. I think this PR should be the first PR post-release, rather than the last PR pre-release.

@cretz
Copy link
Member

cretz commented Oct 8, 2024

I agree. I think this PR should be the first PR post-release, rather than the last PR pre-release.

👍 It would have been nice to get temporalio/sdk-core#818 and temporalio/sdk-core#815 and some others in this release, but I understand that reviewing this core change may take longer than we're wanting for some pure Python changes pending release.

@dandavison dandavison marked this pull request as draft October 8, 2024 16:22
@Sushisource
Copy link
Member

My intention was that this upgrade should also include the changes to avoid applying jobs in different sets and thus match the behavior mentioned in #606

Having all the SDKs work the same way in that regard will be a long term win I think, and is in general just a bit simpler to think about.

@cretz
Copy link
Member

cretz commented Oct 16, 2024

(added #671 as being closed by this)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Fix tonic Vulnerability by Updating Rust Package to Version 0.12.3
3 participants