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

Worker client replacement #517

Merged
merged 2 commits into from
May 3, 2024
Merged

Worker client replacement #517

merged 2 commits into from
May 3, 2024

Conversation

cretz
Copy link
Member

@cretz cretz commented May 3, 2024 •
edited
Loading

What was changed

  • Update core to get Worker client replacement sdk-core#721
  • Added client property on Worker w/ getter/setter
    • Setter now replaces the worker client
  • Added test proving the worker switches servers (even if this is not how people should use client replacement)

Checklist

  1. Closes [Feature Request] Client replacement in worker #513

@cretz cretz requested a review from a team as a code owner May 3, 2024 20:37
@cretz cretz force-pushed the worker-replace-client branch from 31d8578 to d814836 Compare May 3, 2024 20:54
Sushisource
Sushisource approved these changes May 3, 2024
View reviewed changes
temporalio/bridge/src/worker.rs Outdated Show resolved Hide resolved
temporalio/worker/_worker.py
Comment on lines +744 to +745
if bridge_client.config.lazy:
raise RuntimeError("Lazy clients cannot be used for workers")
Copy link
Member

Choose a reason for hiding this comment

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

Seems a little surprising for someone to run into but the structure does make this annoying to deal with.

I guess you would have to asyncify everything from here and up which would also suck.

Copy link
Member Author

@cretz cretz May 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

👍 This check and TODO has always been there, just moved to a separate methods. But yeah, technically we allow people to instantiate unconnected clients and this just prevents those from being used with workers.

@cretz cretz merged commit 0687151 into temporalio:main May 3, 2024
12 checks passed
@cretz cretz deleted the worker-replace-client branch May 3, 2024 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sushisource Sushisource Sushisource approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] Client replacement in worker
2 participants
@cretz @Sushisource