eclipse-archived / smarthome

Eclipse SmartHome™ project
https://www.eclipse.org/smarthome/
Eclipse Public License 2.0
862 stars 787 forks source link

RuleStatus "Running" not sufficient #3474

Open BenediktNiehues opened 7 years ago

BenediktNiehues commented 7 years ago

As user I would assume that if the Rule is in status "RUNNING", that currently the actions are performed. But it is also "RUNNING" if the conditions are evaluated. Maybe there should be StatusDetails like "EvaluatingConditions" and "ExecutingActions" accordingly. Furthermore we have to think about the ruleStatus handling for long running (asynchronous) actions..

0nse commented 7 years ago

Furthermore we have to think about the ruleStatus handling for long running (asynchronous) actions..

To make this more clear: Assuming there were additional status EVALUATING and EXECUTING, EXECUTING would only be a briefly active status.

However, the actual, physical actions are likely to take longer until they have actually finished as they may depend on capabilities of some hardware. Thus, it would be great, if rules were EXECUTING for until the actual actions have been completed as opposed to the ESH actions having been fired.

danchom commented 7 years ago

If we add more states and corresponding events for them this will slowdown the RuleEngine. Actually the running state is an internal state which does not permit to run a rule before ending of previous execution. Even I think we should not fire an event for this state because I don't see why it is interesting for the user. The user has to know if his rule is idle (resoled and ready for run) or it is uninitialized (base on some problems defined in status details) and it can't run. Why the user has to be notified for each run? Yes this is useful for logging debug messages and check if the rules work as expected.

Thus, it would be great, if rules were EXECUTING for until the actual actions have been completed

I think it is not good idea to block a rule until actual action is executing. What is going on with a new events coming from triggers during this period of time? Yes they are collected in a queue, which must not grow forever. Also some of queue events may want to cancel or change current operation immediately. For me the rule has to responds to the trigger events as quickly as possible and it must be a realtime system. Of course you can implement an action handler which finish its execution when the actual action is executed.

0nse commented 7 years ago

we should not fire an event for this state because I don't see why it is interesting for the user

In the past, we witnessed several users complaining about not being able to see when rules execute. Of course, the tech-savvy ESH user is most likely aware of their rules and their effects. However, there are also users that do not have them all in mind and wonder about how the state of some devices has been achieved. That is why we were asked whether a user can inspect when which rules run/ran.

So, if ESH wants to target the casual user, I believe that this information can be of value for some.

I think it is not good idea to block a rule until actual action is executing

I see your points and for now would agree.

danchom commented 7 years ago

That is why we were asked whether a user can inspect when which rules run/ran.

I think the users want to see history of rule execution, which is available in logs, instead of actually rule execution (the running state arrears for very small period of time) As I understand that at the moment there is no way see if the actions are executed or not, because the condition can be unsatisfied. May be we can extends the rule states with the following ones: unsatisfied and executing. The running state will be only for RE internal usage and it will not fire an event.

when a rule is in running state: if the conditions are satisfied the rule goes into executing state and fire event for this state else the rule goes into unsatisfied state and fire event for it.

After the executing or unsatisfied state the rules go back into idle state. In that way the number of events will stay same, because we will replace the running event with the executing or unsatisfied event.

Unfortunately this is backward incompatible change and can break existing code. @kaikreuzer, @sjka, @maggu2810 what do you think?

adimova commented 7 years ago

May I propose the present "running" event to be fired when the result of evaluation of the conditions is known and the event to contain this information as property, this will be a backward compatible change?

danchom commented 7 years ago

You mean not to change the current states, but to delay the Running state event until the result of condition evaluation and add this result into the running event. Yes this will be backward compatible. I think this is good proposal.

danchom commented 7 years ago

@kaikreuzer, @sjka, @maggu2810 , what do you think? Do we have to delay the Running state event until end of condition calculation and add RuleStatusDetails=UNSATISFIED, when the condition check was not passed. The proposal is to delay only the status event for the running state, not to change the states themselves. Rule will go into running state immediately when the rule is triggered.

maggu2810 commented 7 years ago

Publishing every state (running:triggered, running:check-condition, running:execute-actions) is IMHO to noisy for rules.

I don't know if we need a general concept to publish the rules running state as events at all. I would be interested in the opinions of the others...

If some users want to get a feedback about the execution, I think there are several ways to give some feedback (a special action that that publish an item state, log some information, ...).

But if we agree that this is interesting for the most users and events for every rule does not pollute the system, ...

kaikreuzer commented 7 years ago

I also agree with @danchom and @maggu2810 that several events per execution would be too noisy and of no real use.

My assumption actually is that conditions should always be processed VERY quickly and not delay the execution. If a trigger triggers, it will expect that the actions are executed immediately and (3rd-party) conditions should not delay this.

But thinking about this, I actually agree with @BenediktNiehues that it is not good to already say a rule is RUNNING as long as its conditions aren't yet evaluated. If we consider a condition that says that only every 1000th triggered rule should be executed, we would see a lot of status flickering although nothing really happens - so we should only show RUNNING once we reach the action executions. For me it would be ok to keep them in IDLE status until then.

sjsf commented 7 years ago

I agree so far, conditions should not be considered as "RUNNING" - from a user's perspective.

However, if I understood it correctly, a rule will only be in "RUNNING" status until the action handler returns, i.e. long-running stuff will not be covered anyway. Maybe I'm the only one here, but to me this feels confusing. That I'd rather see nothing flickering in the UI if it is lying to me anyway.

sjsf commented 7 years ago

Oh, I just learned that rule actions in fact are executed synchronously. So please forget my above comment.

danchom commented 7 years ago

We have internal meeting and have following proposal: Add new TRIGGERED state which will be activated immediately after the idle state. An event will be fired for this state and it will contain a map of triggered data. The RUNNING state will follow the triggered state only when the conditions are satisfied and actions are executed. Otherwise the rule will go into IDLE state. The state graph will be: IDLE -> TRIGGERED –(satisfied)-> RUNNING -> IDLE IDLE -> TRIGGERED –(not satisfied)-> IDLE This approach will help us to implement CEP trigger base on the outputs of other triggers without adding TriggerChangeListener as I have proposed in #3453.

danchom commented 7 years ago

@kaikreuzer, @maggu2810 , @sjka what do you think about this propolsal?

0nse commented 7 years ago

@danchom this would suffice for our use cases

kaikreuzer commented 7 years ago

@danchom Sounds like an acceptable compromise to me, thanks!

danchom commented 7 years ago

Implemented by the PR #3671

danchom commented 6 years ago

Could you please check the PR #3671 for this issue.