Activity pause/unpause #1729
Activity pause/unpause #1729
Conversation
| @@ -30,7 +30,7 @@ export interface MockActivityEnvironmentOptions { | |||
| * will immediately be in a cancelled state. | |||
| */ | |||
| export class MockActivityEnvironment extends events.EventEmitter { | |||
| public cancel: (reason?: any) => void = () => undefined; | |||
| public cancel: (reason?: any, details?: any) => void = () => undefined; | |||
There was a problem hiding this comment.
What would a MockActivityEnvironment user be expected to provide as input for the details argument? The signature says any, and the ActivityCancellationDetails class has a private constructor, so it looks like it is simply impossible for users to provide anything useful here.
One solution would be to have ActivityCancellationDetails be just an interface, rather than a class, but then we'd have to make all properties optional, as adding more fields in the future would be a backward incompatible change.
Can you please look at what we do with this in other SDKs, and try to figure out a proper API for this?
And btw, reason being any also feels wrong.
packages/activity/src/index.ts
Outdated
| /** | ||
| * Holder object for activity cancellation details | ||
| */ | ||
| public readonly cancellationDetails: ActivityCancellationDetailsHolder; |
There was a problem hiding this comment.
I understand why you introduced this holder. I'm not convinced this is the best solution. An alternative would have been to split Context into an interface AND an implementation class. We'd obviously need to keep to export a Context object, to keep the current() static method, but that wouldn't be hard to do. But whatever, there's nothing wrong with the holder approach.
My real concern is that we're making that indirection be part of the public Context API. That's not desirable from an API perspective, and totally avoidable by keeping that field private, and exposing a cancellationDetails property getter instead.
There was a problem hiding this comment.
I've made this private and provided a getter.
I'm in favour of this simple change to begin with, if the introduction of shared reference mutable fields grows, we can opt to split Context into interface + class impl.
|
I haven't compared with what we did in other SDKs, but it looks like there was quite a few discussions while you were working on the corresponding Python PR. Are you sure this PR correctly mirrors the final state of your Python one? |
Yes, this mirrors the Python activity pause change |
…on details field in activity context private, make activity cancellation tests less flaky
What was changed
Added support to pause/unpause an activity. Cancellations due to pause are received through the heartbeat mechanism of the activity.
Why?
Part of activity operator work.
Closes Heartbeating activities should be interrupted when the activities are paused. #1665
How was this tested:
Couple integration tests
Any docs updates needed?
Likely