search

lyft / presto-gateway

A load balancer / proxy / gateway for prestodb
Apache License 2.0
358 stars 156 forks source link

Share routing rules among all requests #211

Open deigote opened 1 year ago

deigote commented 1 year ago

Small performance improvement plus it very likely closes #166 (it solved it for us at least).

By placing the Rules creation inside the lambda, the previous implementation required to read the routing rules config file for each request, adding unnecessary latency. More importantly, it was not thread-safe, as the rulesEngine and ruleFactory were shared among all requests, only the Rules creation wasn't.

The ruleFactory is of class MVELRuleFactory, which is stateful - it has RuleDefinitionReader reader and ParserContext parserContext as private members. Creating the Rules changes that state, so it should not be shared between concurrent threads. Rules OTOH is a read-only class after it's created (it's just iterated over by the DefaultRulesEngine). Given that the rules are the same for all requests, and that the Rules usage is thread-safe, it makes sense to share it across all of them.

Since deploying this our profiling tool stopped showing the RoutingGroupSelector (which was consuming around 1-2% before) and we stopped seeing the error described in https://github.com/lyft/presto-gateway/issues/166 .

awdavidson commented 1 year ago

I believe this change will require the following test to be updated: https://github.com/deigote/presto-gateway/blob/share-routing-rules-among-all-queries/gateway-ha/src/test/java/com/lyft/data/gateway/ha/router/TestRoutingGroupSelector.java#L85

Given the change in the rules file, the gateway would need restarting?