You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@unomi.apache.org by GitBox <gi...@apache.org> on 2021/07/05 13:51:44 UTC
[GitHub] [unomi] sergehuber opened a new pull request #321: UNOMI-188 Rule event type optimization
sergehuber opened a new pull request #321:
URL: https://github.com/apache/unomi/pull/321
- New optimization for rules: rule conditions are parsed to determine the event types they handle. This is done using a new ParserHelper method that navigates the tree of conditions to resolve which eventTypeCondition are used.
- Settings to deactivate the new optimization in case it causes issues
- Integration tests to validate that the parsing of conditions is behaving as expected
- Performance tests to validate the performance improvement of the optimization
--
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
[GitHub] [unomi] jkevan commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
jkevan commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r665252309
##########
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.
Also it make any private variable readable, this is an anti pattern and Java Object security issue.
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
[GitHub] [unomi] jkevan commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
jkevan commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r665241316
##########
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 rely on the Timer this is very important.
--
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
[GitHub] [unomi] jkevan commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
jkevan commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r672132988
##########
File path: services/src/main/java/org/apache/unomi/services/impl/ParserHelper.java
##########
@@ -145,5 +154,49 @@ public static void resolveValueType(DefinitionsService definitionsService, Prope
interface ConditionVisitor {
void visit(Condition condition);
+ void postVisit(Condition condition);
Review comment:
the postVisit is never called, it should be called in the "visitConditions".
##########
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:
Still not fan of updating a local internal variable inside this getter.
it would be better to do it inside the refreshRules.
##########
File path: services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java
##########
@@ -96,6 +100,10 @@ public void setRulesStatisticsRefreshInterval(Integer rulesStatisticsRefreshInte
this.rulesStatisticsRefreshInterval = rulesStatisticsRefreshInterval;
}
+ public void setOptimizedRulesActivated(Boolean optimizedRulesActivated) {
+ this.optimizedRulesActivated = optimizedRulesActivated;
+ }
+
Review comment:
This seem's not used anymore.
--
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
[GitHub] [unomi] sergehuber commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r667742935
##########
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 will change to use the postVisit.
--
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
[GitHub] [unomi] jkevan commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
jkevan commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r672132988
##########
File path: services/src/main/java/org/apache/unomi/services/impl/ParserHelper.java
##########
@@ -145,5 +154,49 @@ public static void resolveValueType(DefinitionsService definitionsService, Prope
interface ConditionVisitor {
void visit(Condition condition);
+ void postVisit(Condition condition);
Review comment:
the postVisit is never called, it should be called in the "visitConditions".
##########
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:
Still not fan of updating a local internal variable inside this getter.
it would be better to do it inside the refreshRules.
##########
File path: services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java
##########
@@ -96,6 +100,10 @@ public void setRulesStatisticsRefreshInterval(Integer rulesStatisticsRefreshInte
this.rulesStatisticsRefreshInterval = rulesStatisticsRefreshInterval;
}
+ public void setOptimizedRulesActivated(Boolean optimizedRulesActivated) {
+ this.optimizedRulesActivated = optimizedRulesActivated;
+ }
+
Review comment:
This seem's not used anymore.
--
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
[GitHub] [unomi] sergehuber commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r672209259
##########
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:
I see what you mean but the getAllRules method was private and only used in refreshRules anyway. But in order to prevent any misuse I will refactor this anyway.
--
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
[GitHub] [unomi] sergehuber commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r672209571
##########
File path: services/src/main/java/org/apache/unomi/services/impl/ParserHelper.java
##########
@@ -145,5 +154,49 @@ public static void resolveValueType(DefinitionsService definitionsService, Prope
interface ConditionVisitor {
void visit(Condition condition);
+ void postVisit(Condition condition);
Review comment:
Oops... sorry about that. Strange that the tests didn't catch that.
--
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
[GitHub] [unomi] jkevan commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
jkevan commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r665228903
##########
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.
WARNING: will duplicate the Rule in memory
--
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
[GitHub] [unomi] sergehuber commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r672209571
##########
File path: services/src/main/java/org/apache/unomi/services/impl/ParserHelper.java
##########
@@ -145,5 +154,49 @@ public static void resolveValueType(DefinitionsService definitionsService, Prope
interface ConditionVisitor {
void visit(Condition condition);
+ void postVisit(Condition condition);
Review comment:
Oops... sorry about that. Strange that the tests didn't catch that.
##########
File path: services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java
##########
@@ -96,6 +100,10 @@ public void setRulesStatisticsRefreshInterval(Integer rulesStatisticsRefreshInte
this.rulesStatisticsRefreshInterval = rulesStatisticsRefreshInterval;
}
+ public void setOptimizedRulesActivated(Boolean optimizedRulesActivated) {
+ this.optimizedRulesActivated = optimizedRulesActivated;
+ }
+
Review comment:
Yes it is, it is used by Blueprint configuration.
--
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
[GitHub] [unomi] sergehuber commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r665294645
##########
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:
Yes I thought about a postVisit (I was experimenting with enter/leave pattern) but I thought the stack could be useful to more visitor implementations. But if we use a postVisit then it's pretty similar.
--
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
[GitHub] [unomi] sergehuber commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r665296004
##########
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 is actually reproducing something we already have at the PersistenceService level (similar pattern). It is solely done to be able to use it in integration tests and the idea was to avoid polluting the service API with setters. Another of implementing this would be to manually set the value by only allowing specific settings. I'm not sure what's safer at that level.
--
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
[GitHub] [unomi] sergehuber commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r672210138
##########
File path: services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java
##########
@@ -96,6 +100,10 @@ public void setRulesStatisticsRefreshInterval(Integer rulesStatisticsRefreshInte
this.rulesStatisticsRefreshInterval = rulesStatisticsRefreshInterval;
}
+ public void setOptimizedRulesActivated(Boolean optimizedRulesActivated) {
+ this.optimizedRulesActivated = optimizedRulesActivated;
+ }
+
Review comment:
Yes it is, it is used by Blueprint configuration.
--
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
[GitHub] [unomi] sergehuber merged pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
sergehuber merged pull request #321:
URL: https://github.com/apache/unomi/pull/321
--
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
[GitHub] [unomi] sergehuber commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r667741895
##########
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:
Yes you are right I will remove it. However it should not duplicate the rule since we use Sets and Rule inherits from Item that defines the equals() and hashcode() methods to be used with sets.
--
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
[GitHub] [unomi] jkevan commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by 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
[GitHub] [unomi] sergehuber commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r667742762
##########
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:
I will implement the (cleaner) solution of using the OSGi configuration admin.
--
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
[GitHub] [unomi] sergehuber commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r672209259
##########
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:
I see what you mean but the getAllRules method was private and only used in refreshRules anyway. But in order to prevent any misuse I will refactor this anyway.
##########
File path: services/src/main/java/org/apache/unomi/services/impl/ParserHelper.java
##########
@@ -145,5 +154,49 @@ public static void resolveValueType(DefinitionsService definitionsService, Prope
interface ConditionVisitor {
void visit(Condition condition);
+ void postVisit(Condition condition);
Review comment:
Oops... sorry about that. Strange that the tests didn't catch that.
##########
File path: services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java
##########
@@ -96,6 +100,10 @@ public void setRulesStatisticsRefreshInterval(Integer rulesStatisticsRefreshInte
this.rulesStatisticsRefreshInterval = rulesStatisticsRefreshInterval;
}
+ public void setOptimizedRulesActivated(Boolean optimizedRulesActivated) {
+ this.optimizedRulesActivated = optimizedRulesActivated;
+ }
+
Review comment:
Yes it is, it is used by Blueprint configuration.
--
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
[GitHub] [unomi] sergehuber commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r667742111
##########
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:
Yes I will remove this, you are right.
--
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
[GitHub] [unomi] sergehuber commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r665301867
##########
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:
Yes indeed this is wrong I'll correct it.
--
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
[GitHub] [unomi] sergehuber commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r665303601
##########
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:
Another (cleaner) solution would be do to everything through the OSGi ConfigAdmin but this would require improving the service implementation to support dynamic configuration changes.
--
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
[GitHub] [unomi] sergehuber commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r667740727
##########
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:
I still prefer to make a copy to avoid concurrency issues on the data structures as they are not thread-safe
--
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
[GitHub] [unomi] sergehuber commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r667740259
##########
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:
This is not a typo :) It means submit a bug report.
--
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
[GitHub] [unomi] sergehuber commented on a change in pull request #321: UNOMI-188 Rule event type optimization
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #321:
URL: https://github.com/apache/unomi/pull/321#discussion_r667742542
##########
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:
you are right I will only update in the refreshRules call. thanks for catching this.
--
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