Remove easy-rules #540

willmostly · 2024-10-30T18:07:07Z

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

mosabua · 2024-10-30T18:16:44Z

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 · 2024-10-31T00:14:21Z

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

willmostly · 2024-10-31T03:17:27Z

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",


This PR removes support for composite rules

mosabua · 2024-11-04T18:16:32Z

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

oneonestar · 2024-11-23T01:15:39Z

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 · 2024-11-25T20:57:33Z

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 · 2024-11-27T13:40:08Z

@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.

gateway-ha/src/main/java/io/trino/gateway/ha/router/MVELFileRoutingGroupSelector.java

willmostly · 2024-12-13T17:18:13Z

@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.

gateway-ha/src/main/java/io/trino/gateway/ha/router/FileBasedRoutingGroupSelector.java

gateway-ha/src/main/java/io/trino/gateway/ha/router/MVELRoutingRule.java

andythsu · 2025-01-21T16:48:28Z

Unrelated to this PR, but just want to raise a discussion:

What would be an ideal way to verify these routing rules work? Ideally we want to test

when a request matches a particular rule, does it get routed to the right cluster
verify that the request has the right headers (?)

Some of the core logic may depend on these routing rules, so it's essential to make sure they always work at CI/CD level...

oneonestar · 2025-01-23T02:09:33Z

@andythsu It's a good to have feature. You could open an issue to keep track of it.

oneonestar · 2025-01-23T09:22:50Z

gateway-ha/src/main/java/io/trino/gateway/ha/router/FileBasedRoutingGroupSelector.java

+        implements RoutingGroupSelector
+{
+    private static final Logger log = Logger.get(FileBasedRoutingGroupSelector.class);
+    public static final String RESULTS_ROUTING_GROUP_KEY = "routingGroup";


Why is this public?

The intent is for this to be used in routing rules so that users do not need to know this special string "routingGroup". I updated the rule files in the tests to demonstrate

gateway-ha/src/main/java/io/trino/gateway/ha/router/MVELRoutingRule.java

oneonestar · 2025-01-23T10:05:23Z

gateway-ha/src/test/resources/rules/routing_rules_state.yml

+priority: 0
+condition: "true"
+actions:
+  - |


Is - | required, or is that some kind of coding style?

The - indicates this is an array element, the | indicates the beginning of a block scalar: https://github.com/yaml/yaml-grammar/blob/75083c2aaabd0d7453d72de895666b21021826c1/yaml-spec-1.2.txt#L1689

In this instance I'm using it because it allows me to avoid escaping all of the quotes in lines like

state.get("triggeredRules").contains("airflow") && request.getHeader("X-Trino-Client-Tags") contains "label=special"

Co-Authored-By: Prakhar Sapre <[email protected]>

cla-bot bot added the cla-signed label Oct 30, 2024

willmostly force-pushed the will/mvel-rules branch 2 times, most recently from 2c476fc to e398d98 Compare October 31, 2024 00:32

willmostly commented Oct 31, 2024

View reviewed changes

willmostly force-pushed the will/mvel-rules branch 5 times, most recently from de8b88e to d2d6bfa Compare November 1, 2024 17:10

willmostly changed the title ~~Use MVEL for rule evaluation (WIP - Not for review)~~ Remove easy-rules Nov 1, 2024

willmostly force-pushed the will/mvel-rules branch from d2d6bfa to 3603a80 Compare November 1, 2024 18:05

willmostly requested review from mosabua, ebyhr and oneonestar and removed request for mosabua and ebyhr November 4, 2024 16:54

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

vishalya reviewed Dec 3, 2024

View reviewed changes

gateway-ha/src/main/java/io/trino/gateway/ha/router/MVELFileRoutingGroupSelector.java Outdated Show resolved Hide resolved

willmostly force-pushed the will/mvel-rules branch from 206eab6 to efbe2d1 Compare December 13, 2024 17:01

willmostly requested a review from vishalya December 13, 2024 17:02

willmostly force-pushed the will/mvel-rules branch from efbe2d1 to fe0e040 Compare December 13, 2024 17:05

willmostly force-pushed the will/mvel-rules branch from fe0e040 to e821140 Compare December 13, 2024 17:16

oneonestar reviewed Jan 9, 2025

View reviewed changes

willmostly force-pushed the will/mvel-rules branch from e821140 to edc723d Compare January 9, 2025 21:42

willmostly requested a review from oneonestar January 9, 2025 21:45

willmostly force-pushed the will/mvel-rules branch from edc723d to 7bde45b Compare January 13, 2025 19:54

willmostly force-pushed the will/mvel-rules branch 5 times, most recently from 45d3356 to fa054a2 Compare January 22, 2025 21:04

oneonestar reviewed Jan 23, 2025

View reviewed changes

willmostly force-pushed the will/mvel-rules branch from fa054a2 to d9aef12 Compare January 23, 2025 15:39

willmostly and others added 2 commits January 23, 2025 10:43

Use MVEL for rule evaluation

bccb0ae

Co-Authored-By: Prakhar Sapre <[email protected]>

Use RESULTS_ROUTING_GROUP_KEY in routing rules

72b1330

willmostly force-pushed the will/mvel-rules branch from d9aef12 to 72b1330 Compare January 23, 2025 20:00

willmostly requested a review from oneonestar January 23, 2025 21:50

oneonestar approved these changes Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove easy-rules #540

Remove easy-rules #540

willmostly commented Oct 30, 2024 •

edited

Loading

mosabua commented Oct 30, 2024

mosabua commented Oct 31, 2024

willmostly Oct 31, 2024

mosabua commented Nov 4, 2024

oneonestar commented Nov 23, 2024

vishalya commented Nov 25, 2024 •

edited

Loading

willmostly commented Nov 27, 2024

willmostly commented Dec 13, 2024

andythsu commented Jan 21, 2025 •

edited

Loading

oneonestar commented Jan 23, 2025

oneonestar Jan 23, 2025

willmostly Jan 23, 2025

oneonestar Jan 23, 2025

willmostly Jan 23, 2025

Remove easy-rules #540

Are you sure you want to change the base?

Remove easy-rules #540

Conversation

willmostly commented Oct 30, 2024 • edited Loading

Description

Additional context and related issues

Release notes

mosabua commented Oct 30, 2024

mosabua commented Oct 31, 2024

willmostly Oct 31, 2024

Choose a reason for hiding this comment

mosabua commented Nov 4, 2024

oneonestar commented Nov 23, 2024

vishalya commented Nov 25, 2024 • edited Loading

willmostly commented Nov 27, 2024

willmostly commented Dec 13, 2024

andythsu commented Jan 21, 2025 • edited Loading

oneonestar commented Jan 23, 2025

oneonestar Jan 23, 2025

Choose a reason for hiding this comment

willmostly Jan 23, 2025

Choose a reason for hiding this comment

oneonestar Jan 23, 2025

Choose a reason for hiding this comment

willmostly Jan 23, 2025

Choose a reason for hiding this comment

willmostly commented Oct 30, 2024 •

edited

Loading

vishalya commented Nov 25, 2024 •

edited

Loading

andythsu commented Jan 21, 2025 •

edited

Loading