runabol / piper

piper - a distributed workflow engine
Apache License 2.0
487 stars 86 forks source link

Add task progress capability #21

Closed ccamel closed 6 years ago

ccamel commented 6 years ago

I would like to have the tasks of a job able to report their progress. Indeed, some tasks are actually long running process, and from the point of view of the user, it is valuable to have this feedback. I think it could be a good enhancement for piper.

This PR is a first proposal, fully functional though. I tried to keep it as simple as possible.

I have some questions:

What do you think ?

runabol commented 6 years ago

This looks pretty awesome @ccamel. How Do you feel about using an int (primitive) instead of Float for the getProgress() method? the int can represent a percentage of completion (0-100) and would default to 0.

ccamel commented 6 years ago

@creactiviti

runabol commented 6 years ago

Cool thanks @ccamel. The reason why I think a primitive int would work is because if you look at the TaskStatus enum a task can really only be in one of the following states:

  1. CREATED
  2. STARTED
  3. COMPLETED
  4. CANCELLED
  5. FAILED

it seems reasonable to me that for any one of these a figure of 0-100 would work. What do you think?

ccamel commented 6 years ago

@creactiviti Yes, I see. IMHO, the only case not covered by the range [0-100] is the N/A (indeterminate) progress in situation where a task is unable to report its progress. I'm afraid that 0 may be misleading regarding the real progress of the task.

For instance, through an ui we are not able to display a progress bar with an indeterminate value when consuming the progress from the rest api of piper. In jQuery for instance, the progress bar widget has a special value for that (value is either false or an int value in the range [0-max]).

How can we manage this case? (should we?)

runabol commented 6 years ago

Oh I get what you're saying now. For the sake of simplicity I think I would opt to just have the progress stay at 0 until completed at which point it will turn to 100.

ccamel commented 6 years ago

@creactiviti That's fine with me. I amended the code with b2112ce in accordance with your last comment.

runabol commented 6 years ago

merged. thanks @ccamel. great work.