Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove easy-rules #540

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove easy-rules #540

wants to merge 1 commit into from

Conversation

willmostly
Copy link
Contributor

@willmostly willmostly commented Oct 30, 2024 •
edited
Loading

Description

Remove easy-rules and use mvel directly for rule evaluation This PR closes #527. The existing rule format is retained, with the exception that easy-rules style composite rules have been removed and replaced with a state map that allows passing information between rule executions.

Additional context and related issues

Trino Gateway currently leverages easy-rules to evaluate routing rules against an http request. This PR

  • removes easy-rules due to Remove easyrules #527
  • Defines a rule as the combination of a condition, set of actions, and a priority
  • replicate existing easy-rules functionality: routing rules with local file based storage and the MVEL engine for evaluation

The MVELRoutingRule and FileBasedRoutingGroupSelector replicate the existing easy-rules functionality, minus composite rules. Easy-rules uses mvel as the execution engine, so rule condition and action syntax does not need to be migrated. This PR drops support for composite rules, which may be a breaking change for some users.

An additional state object is made available to allow passing evaluation results from lower priority rules to higher priority rules. This should compensate for the removal of the easy-rules composite rules. For usage examples, see tests and docs.

Database storage for MVEL rules will be added as a follow up.

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x ) Release notes are required, with the following suggested text:
Remove easy-rules library. Breaking change: composite rules are deprecated

@cla-bot cla-bot bot added the cla-signed label Oct 30, 2024
@mosabua
Copy link
Member

mosabua commented Oct 30, 2024

Have not looked but removing one layer of indirection that just adds security issues and a bunch of syntax sugar seems like a good idea.

@mosabua
Copy link
Member

mosabua commented Oct 31, 2024

Can you ask in the maintainers group about any potential suggestion or preference for a rules engine

@willmostly willmostly force-pushed the will/mvel-rules branch 2 times, most recently from 2c476fc to e398d98 Compare October 31, 2024 00:32
willmostly
willmostly commented Oct 31, 2024
View reviewed changes
gateway-ha/src/test/java/io/trino/gateway/ha/router/TestRoutingGroupSelector.java
@@ -67,7 +67,6 @@ static Stream<String> provideRoutingRuleConfigFiles()
String rulesDir = "src/test/resources/rules/";
return Stream.of(
rulesDir + "routing_rules_atomic.yml",
rulesDir + "routing_rules_composite.yml",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR removes support for composite rules

@willmostly willmostly force-pushed the will/mvel-rules branch 5 times, most recently from de8b88e to d2d6bfa Compare November 1, 2024 17:10
@willmostly willmostly changed the title Use MVEL for rule evaluation (WIP - Not for review) Remove easy-rules Nov 1, 2024
@willmostly willmostly requested review from mosabua, ebyhr and oneonestar and removed request for mosabua and ebyhr November 4, 2024 16:54
@mosabua
Copy link
Member

mosabua commented Nov 4, 2024

We should get that into 13 imho .. I wont be able to review this week before Friday

@willmostly willmostly force-pushed the will/mvel-rules branch 2 times, most recently from 18d7e42 to 206eab6 Compare November 13, 2024 21:05
@oneonestar
Copy link
Member

I agree to remove the unmaintained easy-rules and the support for composite rule.
However, I think the abstraction for multi-engine support is overkill at this moment.
It's difficult to make a good abstraction when we only have one case to support.
We can add an abstraction later when we decided to add another engine.

@vishalya
Copy link
Member

vishalya commented Nov 25, 2024 •
edited
Loading

Do the rest of the examples and rules are supported by your 'MVEL adapter'

How to check a user? can you give me an equivalent rule with the changes, I tried the below rule with your branch but could not get to work.

---
name: "user check"
description: "if user is will, route to will-group"
condition: "trinoRequestUser.getUser() == \"will\""
actions:
  - "result.put(\"routingGroup\", \"will-group\")"

@willmostly
Copy link
Contributor Author

@vishalya it is tested with all the rules in https://github.com/willmostly/trino-gateway/tree/will/mvel-rules/gateway-ha/src/test/resources/rules. I think your issue is that trinoRequestUser.getUser() returns an Optional, so that condition will always be false. You can use trinoRequestUser.userExistsAndEquals(\"will\") instead.

@willmostly willmostly requested a review from vishalya December 13, 2024 17:02
@willmostly @prakhar10
Use MVEL for rule evaluation
e821140 
Co-Authored-By: Prakhar Sapre <[email protected]>
@willmostly
Copy link
Contributor Author

@oneonestar ptal, I've refactored to remove abstractions for multi-engine support, with the exception of an interface to define the form of a rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mosabua mosabua Awaiting requested review from mosabua

@oneonestar oneonestar Awaiting requested review from oneonestar

@ebyhr ebyhr Awaiting requested review from ebyhr

@vishalya vishalya Awaiting requested review from vishalya

Assignees
No one assigned
Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove easyrules
4 participants
@willmostly @mosabua @oneonestar @vishalya