-
Notifications
You must be signed in to change notification settings - Fork 87
Add attributes needed by server to propagate nexus tasks to worker #711
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
base: master
Are you sure you want to change the base?
Conversation
76cff4d to
d940f4e
Compare
d940f4e to
6fb0a9a
Compare
6fb0a9a to
3425c98
Compare
cretz
left a comment
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.
No big blockers, though I noticed the "Server PR" section of the PR template was removed from the description of this PR. I do think we should wait for most of server impl to understand this has everything needed (no use merging before then).
98f7a60 to
61952c9
Compare
cretz
left a comment
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.
LGTM, but mentioned in last review, the "Server PR" section removed from description template, would like to see implementation get a bit further and linked if possible
Chad, How can I make progress on the server PRs without submitting this API change? I might have asked this before, but what is the recommended protocol? Should I have started the server PRs in my own repo (forked) and have that reference this unsubmitted
I added the server PR to the description. temporalio/temporal#9231 |
This enables the server to track which worker is executing eager-dispatched activities, allowing activity cancellation to be routed correctly.
61952c9 to
98350a9
Compare
Yeah, basically this. It just means you have a PR mostly done but referencing your API branch instead of API master. It's just a good way to make sure we don't "oh yeah" during implementation (even when we think we're so sure what we want from he API). May be able to ask server peers on common approach here, e.g. feature branches. |
What changed?
Added worker_control_task_queue to poll requests:
Note: worker_instance_key was already added to these requests in Enhance ShutdownWorkerRequest and poll calls with worker_instance_key #686.
Added worker_instance_key and worker_control_task_queue to:
Why?
To enable server to send nexus tasks to worker.
Example use case:
Breaking changes: None
Server PR