[GitHub] [unomi] jkevan commented on a change in pull request #321: UNOMI-188 Rule event type optimization

[GitBox <gi...@apache.org>]

jkevan commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r665186828



##########
File path: services/src/main/resources/org.apache.unomi.services.cfg
##########
@@ -69,3 +69,8 @@ rules.statistics.refresh.interval=${org.apache.unomi.rules.statistics.refresh.in
 
 # The indicator should be checked is there a sourceId in the system or not
 events.shouldBeCheckedEventSourceId=${org.apache.unomi.events.shouldBeCheckedEventSourceId:-false}
+
+# If this setting is active, the rules engine will try to classify the events by event type internally which makes
+# rules execution a lot faster. If there are any problems detected with rules execution, you might want to try to turn
+# off the optimization and file a bug report if this fixed the problem.

Review comment:
       Typo "file a bug report"

##########
File path: services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java
##########
@@ -157,9 +165,20 @@ private void loadPredefinedRules(BundleContext bundleContext) {
         Boolean hasEventAlreadyBeenRaisedForSession = null;
         Boolean hasEventAlreadyBeenRaisedForProfile = null;
 
-        List<Rule> allItems = allRules;
+        Set<Rule> eventTypeRules = new HashSet<>(allRules); // local copy to avoid concurrency issues
+        if (optimizedRulesActivated) {
+            eventTypeRules = rulesByEventType.get(event.getEventType());
+            if (eventTypeRules == null || eventTypeRules.isEmpty()) {
+                return matchedRules;
+            }
+            eventTypeRules = new HashSet<>(eventTypeRules);

Review comment:
       This resignation seem's not necessary `eventTypeRules` already equals to `rulesByEventType.get(event.getEventType())`

##########
File path: services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java
##########
@@ -348,6 +379,7 @@ public void setRule(Rule rule) {
         if (condition != null) {
             if (rule.getMetadata().isEnabled() && !rule.getMetadata().isMissingPlugins()) {
                 ParserHelper.resolveConditionType(definitionsService, condition, "rule " + rule.getItemId());
+                updateRulesByEventType(rulesByEventType, rule);

Review comment:
       Originally we do not update the allRules, why do we update the rulesByEventType ?
   It means that the allRules is updated by the Timer (refreshRules)
   but rulesByEventType is updated directly here then updated again by the Time (refreshRules).
   Is it on purpose ?
   
   The reason I asked is mainly that it introduce differences on the process timing.
   setRule: will register the rule instantly in the rulesByEventType: instant
   removeRule: will remove the rule using the timer to refresh the rulesByEventType: delayed
   
   I dont like this differences.
   either we update everythings instantly, either we relay on the timer.
   But mixing both could bring confusion.
   
   ALSO be warning here, this could have bad impacts:
   - because this is instantly done, in case the rule already exists it will duplicate the Rule in the memory, because the rule will alraedy exists in "rulesByEventType", the updateRulesByEventType is not checking that the rule is already present, so it will be duplicated. BUT the Timer will fix this.
   - This is exactly why we should not mix the instant and delayed operations.
   - We should only relay on the Timer this is very important.

##########
File path: services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java
##########
@@ -157,9 +165,20 @@ private void loadPredefinedRules(BundleContext bundleContext) {
         Boolean hasEventAlreadyBeenRaisedForSession = null;
         Boolean hasEventAlreadyBeenRaisedForProfile = null;
 
-        List<Rule> allItems = allRules;
+        Set<Rule> eventTypeRules = new HashSet<>(allRules); // local copy to avoid concurrency issues
+        if (optimizedRulesActivated) {
+            eventTypeRules = rulesByEventType.get(event.getEventType());
+            if (eventTypeRules == null || eventTypeRules.isEmpty()) {
+                return matchedRules;

Review comment:
       This return statement doesnt seem's corect, as it will ignore `rulesByEventType.get("*")`.
   Even if the current event doesnt match any specific rules it could match more complex rules provided by `rulesByEventType.get("*").`

##########
File path: services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java
##########
@@ -240,12 +259,23 @@ private void updateRuleStatistics(RuleStatistics ruleStatistics, long ruleCondit
         allRuleStatistics.put(ruleStatistics.getItemId(), ruleStatistics);
     }
 
+    public void refreshRules() {
+        try {
+            allRules = getAllRules();
+        } catch (Throwable t) {
+            logger.error("Error loading rules from persistence back-end", t);
+        }
+    }
+
     private List<Rule> getAllRules() {
         List<Rule> allItems = persistenceService.getAllItems(Rule.class, 0, -1, "priority").getList();
+        Map<String,Set<Rule>> newRulesByEventType = new HashMap<>();
         for (Rule rule : allItems) {
             ParserHelper.resolveConditionType(definitionsService, rule.getCondition(), "rule " + rule.getItemId());
+            updateRulesByEventType(newRulesByEventType, rule);
             ParserHelper.resolveActionTypes(definitionsService, rule);
         }
+        this.rulesByEventType = newRulesByEventType;

Review comment:
       For code quality and readability I would suggest to not modified internal in memory object "this.rulesByEventType" inside a function call "get...".
   It would be more clean to do this in the refreshRules(), as the refreshRules already modified the this.allRules.
   But it would introduce a secondary loop. So I let you decide on this one.
   But I'm not fan of updating internal variables in this function, because this function was originally made to return the list of complete rules, ideally it should not contains internal additional logic.

##########
File path: services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java
##########
@@ -335,6 +365,7 @@ public Rule getRule(String ruleId) {
         Rule rule = persistenceService.load(ruleId, Rule.class);
         if (rule != null) {
             ParserHelper.resolveConditionType(definitionsService, rule.getCondition(), "rule " + rule.getItemId());
+            updateRulesByEventType(rulesByEventType, rule);

Review comment:
       Seem's strange to update in memory rulesByEventType when reading a rule object, the update is done during rule registration, I dont see what could make the rule modified here that would require to update the memory list of rules by events.

##########
File path: services/src/main/java/org/apache/unomi/services/impl/ParserHelper.java
##########
@@ -63,39 +63,41 @@ public void visit(Condition condition) {
                     }
                 }
             }
-        });
+        }, new Stack<>());
         return result.isEmpty();
     }
 
     public static List<String> getConditionTypeIds(Condition rootCondition) {
         final List<String> result = new ArrayList<String>();
         visitConditions(rootCondition, new ConditionVisitor() {
             @Override
-            public void visit(Condition condition) {
+            public void visit(Condition condition, Stack<String> conditionTypeStack) {
                 result.add(condition.getConditionTypeId());
             }
-        });
+        }, new Stack<>());
         return result;
     }
 
-    private static void visitConditions(Condition rootCondition, ConditionVisitor visitor) {
-        visitor.visit(rootCondition);
+    private static void visitConditions(Condition rootCondition, ConditionVisitor visitor, Stack<String> conditionTypeStack) {

Review comment:
       I like the idea of the Stack, but I think it could be more generic that this.
   
   We already have a ConditionVisitor interface, ideally the visitor should handle his own stack in case it need one.
   Because right now this stack is imposed to all condition visitor, but it's only relevant for the EventTypeConditionVisitor. Because of this, it will be completely useless for any other visitors.
   
   Like the getConditionTypeIds(), it will build a stack of conditionType, but it will not be used.
   
   The stack should be internal to the EventTypeConditionVisitor directly.
   And if it's required we should modify the ConditionVisitor interface with a new method: postVisit.
   
   So it will look like this for the EventTypeConditionVisitor:
   new EventTypeConditionVisitor: internalStack = new Stack()
   visit: internalStack.push(condition.getConditionTypeId())
   postVisit: internalStack.pop()
   

##########
File path: services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java
##########
@@ -381,15 +413,23 @@ public void removeRule(String ruleId) {
         persistenceService.remove(ruleId, Rule.class);
     }
 
+    @Override
+    public void setSetting(String fieldName, Object value) throws NoSuchFieldException, IllegalAccessException {
+        Field field = this.getClass().getDeclaredField(fieldName);
+        field.set(this, value);
+    }
+
+    @Override
+    public Object getSetting(String fieldName) throws NoSuchFieldException, IllegalAccessException {
+        Field field = this.getClass().getDeclaredField(fieldName);
+        return field.get(this);
+    }
+

Review comment:
       This look overkill and dangerous as any in memory variables could be overrided in the service. And it seem's to be used only by the IT test to be able to set the optimizedRules to false.
   with this opened function it's possible to change the value of any internal variables in the current service:
    
       private BundleContext bundleContext;
       private PersistenceService persistenceService;
       private DefinitionsService definitionsService;
       private EventService eventService;
       private SchedulerService schedulerService;
       private ActionExecutorDispatcher actionExecutorDispatcher;
       private List<Rule> allRules;
       private Map<String,RuleStatistics> allRuleStatistics = new ConcurrentHashMap<>();
       private Integer rulesRefreshInterval = 1000;
       private Integer rulesStatisticsRefreshInterval = 10000;
   
   So it could be dangerous.
   
   Instead we could open the setOptimizedRulesActivated in the interface, because it's the only one required for the IT test.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@unomi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org