eventual icon indicating copy to clipboard operation
eventual copied to clipboard

feat: activity override and cancellation

Open thantos opened this issue 3 years ago • 10 comments

closes #97

  • ~~Cancel~~, Complete, or Fail any activity from event handler, activity, api, or workflow using it's activity token.
  • Cancel, ~~Complete, or Fail~~ an activity reference the workflow
  • Activity can check to see if it is "closed" by using the heartbeat call, which will return {closed: true} when the activity or the workflow is considered resolved/finished/closed.
  • Fixed a bug that would cause only an error's name to be output.

thantos avatar Dec 13 '22 04:12 thantos

I'm a bit skeptical that it's a good idea to be able to override an activity's result, beyond cancellation. Wouldn't this make activities a lot less 'does what it says on the tin'? With this I'm seeing them as akin to functions that could return anything based on what's intercepting them, and so are harder to reason about. What would the use cases for this be?

cfraz89 avatar Dec 13 '22 05:12 cfraz89

The behavior is the same mechanism as "async activities".

I'm certain we need the ability to cancel, which is essentially the same as failing.

Which led to the current state.

Developers can do whatever they want, but in most cases they will only be able to allow others to override when the activity token is distributed, which is up to the activity.

thantos avatar Dec 13 '22 05:12 thantos

The behavior is the same mechanism as "async activities".

I'm certain we need the ability to cancel, which is essentially the same as failing.

Which led to the current state.

Developers can do whatever they want, but in most cases they will only be able to allow others to override when the activity token is distributed, which is up to the activity.

Ok makes sense

cfraz89 avatar Dec 13 '22 09:12 cfraz89

The behavior is the same mechanism as "async activities".

I'm certain we need the ability to cancel, which is essentially the same as failing.

Which led to the current state.

Developers can do whatever they want, but in most cases they will only be able to allow others to override when the activity token is distributed, which is up to the activity.

I agree with Chris, I don't think we should be adding the override functionality. Just because we can doesn't mean we should. Let requirements drive features. Please remove it. We only need the ability to complete an activity by token, not arbitrarily set the value.

sam-goodwin avatar Dec 13 '22 12:12 sam-goodwin

The behavior is the same mechanism as "async activities". I'm certain we need the ability to cancel, which is essentially the same as failing. Which led to the current state. Developers can do whatever they want, but in most cases they will only be able to allow others to override when the activity token is distributed, which is up to the activity.

I agree with Chris, I don't think we should be adding the override functionality. Just because we can doesn't mean we should. Let requirements drive features. Please remove it. We only need the ability to complete an activity by token, not arbitrarily set the value.

There is no difference between complete activity and this functionality. Override cannot override the output of a completed/failed activity, but it can complete an uncompleted activity.

Is the word override confusing? I had called it "finish", but that was also confusing with "complete".

thantos avatar Dec 13 '22 16:12 thantos

In my slack pr I implemented a complete api. Is it different than that?

sam-goodwin avatar Dec 13 '22 17:12 sam-goodwin

In my slack pr I implemented a complete api. Is it different than that?

Ok, you had implemented completeActivity from an activity function. This is the same as calling my completeActivity intrinsic from anywhere except from within a workflow.

Issues with the current state in main:

  1. Missing the matching failActivity API
  2. Calling activity.complete can actually complete any activity and it is not possible to know what activity a token is for from the token itself. So I can see the value in that it is typesafe, just doesn't do anything at runtime.
  3. activity.complete is not safe to be called from within a workflow. It makes a network call and is not durable.

In this PR, I updated activity.complete to be safe within a workflow (durable command).

thantos avatar Dec 13 '22 17:12 thantos

Ok, made changes based on feedback:

  1. removed the ability to complete or fail an activity by reference (const act = myAct(); act.complete(value);) 2. the ability to cancel an activity remains (const act = myAct(); act.cancel(reason);)
  2. removed the ability to "cancel" an activity from outside of the workflow that started it. (cancelActivity(activityToken)) 3. the ability to complete or fail remain (completeActivity(activityToken, value))
  3. Updated the activity.complete to work within a workflow. (const myAct = activity(...); workflow("", (token) => myAct.complete(token, value)))

thantos avatar Dec 13 '22 17:12 thantos

I still don't understand what the use case is for this outside of just reporting success/failure for an async activity. Why does a workflow need to be able to cancel an activity? I worry we are adding too much breadth prematurely.

sam-goodwin avatar Dec 13 '22 17:12 sam-goodwin

Now supported

const myAct = activity("myAct", () => { ... });

// in an event handle or API
someEvent.on(async (token) => {
    // complete any activity
    await myAct.complete(token, value);
    // complete or fail any activity
    await completeActivity(token, value);
    await failActivity(token, error, message);
})

workflow("", async () => {
    // complete any activity
    await myAct.complete(token, value);
    // complete or fail any activity
    await completeActivity(token, value);
    await failActivity(token, error, message);

    const actInstance = myAct();
    // cancel/fail an activity from it's calling workflow.
    await actInstance.cancel();
    await actInstance // throws ActivityCancelled;
})

// some other lambda code
handler = (token) => {
    await workflowClient.completeActivity(token);
    await workflowClient.failActivity(token);
}

thantos avatar Dec 13 '22 17:12 thantos