Trigger engine code review

[ujhelyiz]

Trigger engine needs review. Issues:

• package naming
• code duplication
• validation framework connection
• lifecycle management
• documentation

[szabta89]

For the record: this first attempt deals with the current state of matcher callbacks. An updated version will be available later which uses the callback branch.

[szabta89]

@abelhegedus Please review the changes and if you find it ok, I will start trying out the callback branch.

[szabta89]

@abelhegedus You will see that I have created an interface for the RuleFactory, but in my opinion it can not be assigned to the RuleEngine itself as it needs the corresponding IncQueryEngine of the Agenda to instantiate rules.

[abelhegedus]

Okay, I think your implementation with the RuleFactory is OK.

Next step, please look at the violations in Sonar and correct them (e.g. using proper loggers):

• https://build.inf.mit.bme.hu/sonar/drilldown/violations/1655
• https://build.inf.mit.bme.hu/sonar/drilldown/violations/507
• https://build.inf.mit.bme.hu/sonar/drilldown/violations/508

I also think that the API classes should be put in an .api package.

[szabta89]

@abelhegedus Please take a look at the new code and on the TriggeredQueryResultMultiMap class, because there were major modifications in the code of the Rule Engine. Please also check whether the JavaDoc is fine now.

[abelhegedus]

Thanks, the new code looks fine. Some comments:

• the TriggeredQueryResultMultimap (TRQM) must be visible from outside, so we have two choices:
  • export the util package and move TriggerEngineUtil to somewhere else
  • move TQRM to an other package (e.g. api), together with other classes that are in util and should be visible
• actually, I think the functionality of TriggerEngineUtil should be somewhere else completely (e.g. patternlanguage.util)
• how does TQRM ensure that the results from an added matcher are added at once, if you commented out the callback? Am I right that it will only update at the next "tick" (afterUpdateCallback)?

[szabta89]

Please place the TRQM class wherever you want. The afterUpdateCallback was commented out because there is no such method anymore. The Agenda is automatically refreshed by the rules and the agenda has a register method which can send the notifications for the first time to the observer. See the interaction between the Rules and Agenda for an example.

[abelhegedus]

Further considerations: the trigger engine uses the managed engine at the moment. Which means that a wipe or dispose will stop the triggerengine from functioning. Thus, the following should be decided:

• if the triggerengine uses a managed engine, it should also register for after wipe callbacks
• the triggerengine could use an unmanaged engine, but this is only safe if it creates the engine itself and does not allow access to it to anyone else!

Since the constructors of Agenda are protected, we will only need to ensure the proper engine handling in the RuleEngine.

Related:

• remove Agenda(Notifier) constructor, noone uses it and the engine will be prepared by the RuleEngine
• remove Agenda,getIqEngine(), and add Agenda.getLogger instead. The only user of the former method is in TRQM, which will use the getLogger instead.

[ujhelyiz]

Meeting log: we should use a managed engine. Further related todos are described in #365.

[abelhegedus]

Some more questions:

• I find it strange, that the Agenda "provides an unmodifiable view for the collection of applicable activations" but it also has a public activationAppeared/Disappeared method due to it implementing IActivationNotificationListener. Is this considered a good design? Maybe Aganda should have an interface class?
• It's really confusing, that users must register "firing strategies" into an agenda but have no way of knowing if a given agenda has one already or not. I think that similarly to the RuleFactory, the RuleEngine should be able to create an agenda with a given type of firing strategy.
• Both the Agenda and the Rule has "allowMultipleFiring". If rules can only enter the Agenda through createRule and it copies this value into the Rule, which uses it to create an Activation that also stores this value... I hope you see what my problem is.
• You completely removed the callbackAfterUpdate from the code, while we discussed that it should be used as the automatic firing strategy "heartbeat".
• The AutomaticFiringStrategy only does something on activationAppeared. I cannot understand this.

[abelhegedus]

As discussed on the meeting, the current state of the trigger engine code will be moved, and other issues will be created for further changes required before making the API public.