Open
Conversation
the previous implementation required to read the routing rules config file for each request, adding unnecessary latency. More importantly, it was done in a non-thread-safe way, as the `rulesEngine` and `ruleFactory` were shared among all requests, but the `Rules` creation was not. \ Since the class MVELRuleFactory is stateful (as it has `RuleDefinitionReader reader` and `ParserContext parserContext` as private members), 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.\ We have deployed this code in production at Datadog and our APM profiling tool shows a reduction of around ~1-2%, plus we stopped seeing the error described in lyft#166
|
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? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Small performance improvement plus it very likely closes #166 (it solved it for us at least).
By placing the
Rulescreation 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 therulesEngineandruleFactorywere shared among all requests, only theRulescreation wasn't.The
ruleFactoryis of classMVELRuleFactory, which is stateful - it hasRuleDefinitionReader readerandParserContext parserContextas private members. Creating theRuleschanges that state, so it should not be shared between concurrent threads.RulesOTOH is a read-only class after it's created (it's just iterated over by theDefaultRulesEngine). Given that the rules are the same for all requests, and that theRulesusage 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 #166 .