[sebastian.s]

Hi Huisheng Xu!

I should have noticed that you've been talking about the outdated version of the patch. So nevermind. The great news are that we can go ahead and apply patchs having a solution for both issues.

Sebastian

[swiderski.maciej]

Hi,

patch uploaded to jira, please review it and let me know your comments.

If it comes to committing I think Alejandro needs to approve it since it is a change of the API. So let's hope he can find some time to look into it as well.

Cheers,

Maciej

[rebody]

Hi Maciej,

  The patch run perfectly.  Thank you for you job.

  There are still something we should review.

  1.Should we modify TaskImpl.isComplete(), let this method fit additional states: 'timeout' and 'cancelled'.

     Now the isComplete() is like this:

public boolean isCompleted() {
    if (Task.STATE_COMPLETED.equals(state)) {
      return true;
    }
    if ((Task.STATE_OPEN.equals(state)) || (Task.STATE_SUSPENDED.equals(state))) {
      return false;
    }
    return true;
  }

    We should add more information in here.  And the state constaints is defined in Task interface.  So should we move STATE_TIMEOUT and STATE_CANCELLED from HistoryTask to Task?

   Actually, the STATE_COMPLETED field in Task is duplicated by STATE_COMPLETED field in HistoryTask.

  2. The content of TaskTimeout.java and TaskCancel.java is exactly the same.  The only difference of them is the completion state,  so should we make a abstract class, e.g. AbstractTaskCancel,  and let them inherit the superclass?

  3. I notice that there is a jbpm.task.lifecycle.xml configution file in the classpath since jBPM-4.0.0-Alpha2,  and it contains the prossible states of task.  Should we modify it as well? or this configuration file is useless and should be removed in the future version?  The content of this file like below:

<task-lifecycle initial="open">
  <state name="open">
    <transition name="complete" to="completed" />
    <transition name="suspend" to="suspended" />
    <transition name="cancel" to="cancelled" />
  </state>
  <state name="suspended">
    <transition name="resume" to="open" />
    <transition name="cancel" to="cancelled" />
  </state>
  <state name="cancelled" />
  <state name="completed" />
</task-lifecycle>

[mwohlf]

Hi Guys,

I also stumbled across the jbpm.task.lifecycle.xml file and tried to find out what it was doing, this is what i managed to figure out so far:

The content of the file is read in org.jbpm.pvm.internal.task.LifeCycle.java, it is parsed and seems to implement a kind of min-process / state machine for the task status. You can call LifeCycle.fireLifeCycleEvent(String eventName, TaskImpl task)and it changes the state of the task according to the transitions defined in jbpm.task.lifecycle.xml.

protected static void fireLifeCycleEvent(String eventName, TaskImpl task) {

    // reading the state machine
    ExecutionImpl lifeCycleExecution = new ExecutionImpl();
    ProcessDefinitionImpl lifeCycleProcess = getLifeCycle(task);
    lifeCycleExecution.setProcessDefinition(lifeCycleProcess);

    // setting up the current state:
    String state = task.getState();
    Activity activity = lifeCycleProcess.getActivity(state);
    lifeCycleExecution.setActivity((ActivityImpl) activity);

    // performing a transition
    lifeCycleExecution.signal(eventName);

    // transfering the new state to the task
    task.setState(lifeCycleExecution.getActivity().getName());
}

I think this is great stuff, unfortunatly I couldn't find any place in the code where LifeCycle.fireLifeCycleEvent(String eventName,  TaskImpl task)is called, but to me it seems a nice idea to have the task status and transitions configurable in a single file like this.

[swiderski.maciej]

Hi,

here comes my comments:

HuiSheng Xu wrote:

   We should add more information in here.  And the state constaints is defined in Task interface.  So should we move STATE_TIMEOUT and STATE_CANCELLED from HistoryTask to Task?

   Actually, the STATE_COMPLETED field in Task is duplicated by STATE_COMPLETED field in HistoryTask.

In my opinion Task and HistoryTask are two different entities and they should be kept separated.

States are defined in both places because they are used by different queries. So I think they should be as is.

HuiSheng Xu wrote:

  1.Should we modify TaskImpl.isComplete(), let this method fit additional states: 'timeout' and 'cancelled'.

     Now the isComplete() is like this:

public boolean isCompleted() {
    if (Task.STATE_COMPLETED.equals(state)) {
      return true;
    }
    if ((Task.STATE_OPEN.equals(state)) || (Task.STATE_SUSPENDED.equals(state))) {
      return false;
    }
    return true;
  }

I think this is only internal method and IMO it is about verifying if that was already completed in regular way. Timeout and cancel are not completion states, they just close the task and not complete it. That's why states for them are only for history purposes.

HuiSheng Xu wrote:

  2. The content of TaskTimeout.java and TaskCancel.java is exactly the same.  The only difference of them is the completion state,  so should we make a abstract class, e.g. AbstractTaskCancel,  and let them inherit the superclass?

Yeap, you are right here, they are quite the same. We could make an abstract class or shall we give it some thought and check if they should be the same?!

HuiSheng Xu wrote:

  3. I notice that there is a jbpm.task.lifecycle.xml configution file in the classpath since jBPM-4.0.0-Alpha2,  and it contains the prossible states of task.  Should we modify it as well? or this configuration file is useless and should be removed in the future version?  The content of this file like below:

<task-lifecycle initial="open">
  <state name="open">
    <transition name="complete" to="completed" />
    <transition name="suspend" to="suspended" />
    <transition name="cancel" to="cancelled" />
  </state>
  <state name="suspended">
    <transition name="resume" to="open" />
    <transition name="cancel" to="cancelled" />
  </state>
  <state name="cancelled" />
  <state name="completed" />
</task-lifecycle>

I haven't seen it before, so thanks for pointing it out. I did look for it in the code but with the same result as Michael, no real references of it. They should not be used by custom code either since they are internal classes.

Thanks for your comments

Maciej

[rebody]

Hi Maciej,

  For TaskCancel and TaskTimeout, I think we could reference the TaskDelete.  The constructor of TaskDelete allowed providing a reason for deleting task.  We already have the state in HistoryTask, it is much easy for us to merge the TaskCancel and TaskTimeout into a TaskCancel class and let outside give this history event a reason message.

[swiderski.maciej]

I agree, will do required changes today and produce a patch for it.

[sebastian.s]

Hi Michael,

the task lifecycle is indeed a great idea. It has not yet been used in jBPM 4.x. The ideas shown and implemetend there are for sure the way to go for the future . We have not taken them into account now because at the moment we are focusing on fixing bugs and not implementing new features. In case you have read the request for comments on jBPM 5 you will have noticed that they are taking WS-HumanTask into account which also uses a task lifecycle to deal with tasks.

Cheers

Sebastian

[aguizar]

Maciej, I just read the complete discussion and am about to review the patch. Introducing a separate interface rather than extend the existing one seems like the way to go for me as well. Thanks for the work.

[aguizar]

I reviewed the patch today. It looks OK; however, since timer expiration is mostly a special case of cancel, expiration, cancellation and any other form of premature task termination can be handled through a single cancel(outcome, state) method. I have attached a new patch to JBPM-2537. Let me know what you all think.

On a different topic, I am not quite happy with the addition of the TimedActivityBehaviour interface and Execution.signal(String, boolean) method just to deliver a timer expiration signal. It should be possible to do it through the existing ExternalActivityBehavior and Execution.signal(String, Map) by passing a special parameter that indicates the cancellation state ("cancelled", "expired" or any other). I'm going to explore this path next week.

[rebody]

Hi Alejandro,

  If you merge the timeout() and cancel() methods of the TimeoutActivityBehaviour.  It seems the interface name should change to CancelableActivityBehaviour,  not TimedActivityBehaviour.

[swiderski.maciej]

Alejandro Guizar wrote:

I reviewed the patch today. It looks OK; however, since timer expiration is mostly a special case of cancel, expiration, cancellation and any other form of premature task termination can be handled through a single cancel(outcome, state) method. I have attached a new patch to JBPM-2537. Let me know what you all think.

On a different topic, I am not quite happy with the addition of the TimedActivityBehaviour interface and Execution.signal(String, boolean) method just to deliver a timer expiration signal. It should be possible to do it through the existing ExternalActivityBehavior and Execution.signal(String, Map) by passing a special parameter that indicates the cancellation state ("cancelled", "expired" or any other). I'm going to explore this path next week.

Works fine. Like you changes, they simplify it, so that's great.

I would agree if we are talking only about expiration/cancellation of tasks then perhaps we could skip additional interface. But since there could be some custom activities defined I think there will be a need for distinguishing it (I mean cancel and timeout/expired). In some cases it is important to know if activity was executed manually by signalling or automatically on timeout event.

Looking forward to your exploration results.

[aguizar]

Looking for other perspectives about this issue I read the WS-HumanTask specification. The task life cycle from section 4.7 proposes a single state, Obsolete, for both manual/external skipping (4.7.5) and expiration (section 4.7.6).

With this model in mind I renamed the -cancel methods to -skip and merged STATE_CANCELLED and STATE_EXPIRED into STATE_OBSOLETE. TaskActivity declares "obsolete" a task that is still incomplete upon signal, by calling the skip(outcome) method. The resulting model is clean as it neither requires a new ActivityBehavior subinterface nor changes to the existing ExecutionImpl.signal methods or Signal class, with the one drawback that it does not distinguish between manual cancellation and timer expiration. It is not yet clear to me why is the distinction important though.

I've commited these changes and resolved JBPM-2537. Please take a look at the solution and feel free to reopen the issue if you feel some important aspect was left out.

[swiderski.maciej]

Looks really good. Moreover good to follow standard, especially while bearing in mind v5 that will be based on WS-HT.

If it comes to your question about distinguishing between manual cancellation and timer I would say that it can be important from business point of view.

Since expiration usually means some kind of escalation meaning task (or information that should be gathered within this step) are important for the process and cancellation could be done just to skip certain types of steps in the process and by doing that process can have inconsistent state - no one has completed the task and it was not escalated so some of the information can be missing. Especially that signal can be made by providing any transition from all available.

Perhaps an alternative to have different states/cancellation methods would be to add a task comment while signaling in skip mode?! At least some information why the task was skipped.

[rebody]

Hi Alejandro,

Happy to see this issue finally was resolved.  And we didn't use additional interface in the the jbpm-api.

I must say that I don't like ExternalActivityBehaviour interface, it should be design to handle much more scenarioes, not just for the signal.  Since Tom and Joram just recently release their new BPMN engine - activiti.  I am very interesting in the EventActivityBehaviour in the current distribution of Activiti-5.0-alpha1.  It is worth to refer.