Add activity cancels from nexus#1160
Conversation
| } | ||
| } | ||
| } | ||
| pub mod nexusservices { |
There was a problem hiding this comment.
Does the nexus worker send heartbeat? If it does, then I need to find a way to exclude this from the ListWorkers result.
There was a problem hiding this comment.
~Good question. It shouldn't, if it does I'll make sure it doesn't. ~
Changed my mind. We are already making sure it gets excluded from metrics (since we saw it show up and had that whole discussion re: Nexus usage), and being able to diagnose possible issues with it (getting overloaded, not enough slots, whatever) is useful. Also being dealt with here: temporalio/cli#1019
Sushisource
force-pushed
the
add-activity-cancels-from-nexus
branch
2 times, most recently
from
322f2a4 to
4f0e626
Compare
Sushisource
force-pushed
the
add-activity-cancels-from-nexus
branch
from
4f0e626 to
8bc16e3
Compare
Sushisource
force-pushed
the
add-activity-cancels-from-nexus
branch
4 times, most recently
from
1cfd5f0 to
6379fe0
Compare
Sushisource
force-pushed
the
add-activity-cancels-from-nexus
branch
from
cf86496 to
7d0d454
Compare
yuandrew
left a comment
yuandrew
left a comment
There was a problem hiding this comment.
A few minor things, biggest question is should we update any public/user facing docstrings to mention activity cancellation can now be delivered without activity heartbeats when worker heartbeating is enabled?
| .build(), | ||
| ); | ||
| // Timer needed to avoid cancel-before-sent | ||
| ctx.timer(Duration::from_millis(10)).await; |
There was a problem hiding this comment.
This doesn't actually guarantee the activity has started, right? Only that it's been scheduled? Can we better guarantee that the activity has started?
There was a problem hiding this comment.
It doesn't but that doesn't matter, it only needs to be scheduled here
| .unwrap() | ||
| .into_inner(); | ||
| #[allow(deprecated)] | ||
| #[allow(deprecated)] |
There was a problem hiding this comment.
Lol what. Thanks. I think it was a merge issue.
| identity: self.identity(), | ||
| worker_version_capabilities: self.worker_version_capabilities(), | ||
| deployment_options: self.deployment_options(), | ||
| // TODO: Piggyback worker heartbeats here if this is the system nexus worker and reset |
There was a problem hiding this comment.
We have something tracking this work? Worried this will get lost. If not, i can make one and assign it to myself, I'll need to dig up my old impl for this, I remember doing this like a year ago haha
There was a problem hiding this comment.
We should as part of the overall workstream - but yes please make sure we are somewhere
| ); | ||
| // Timer needed to avoid cancel-before-sent | ||
| ctx.timer(Duration::from_millis(10)).await; | ||
| act_fut.cancel(); |
There was a problem hiding this comment.
It never clicked from me until now that this is all it takes to cancel the activity. Some reason I thought there would need to be special actions you'd have to initiate from the server, or something. Very nice
We certainly will in lang layers but, I don't think there's actually much in core that will need to change. I'll look for any and change them if so (Rust SDK itself perhaps, but, I'm not sure it ever mentioned this limitation anywhere). |
3 participants
Creates a worker which polls on the appropriately-named worker commands nexus task queue, and handles and activity cancellations sent down it.
The worker is not started unless the server supports this capability.