Enable security automaton caching #34028
Merged
Conversation
This file contains 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
Building automatons can be costly. For the most part we cache things that use automatons so the cost is limited. However: - We don't (currently) do that everywhere (e.g. we don't cache role mappings) - It is sometimes necessary to clear some of those caches which can cause significant CPU overhead and processing delays. This commit introduces a new cache in the Automatons class to avoid unnecesarily recomputing automatons.
tvernum
added
>enhancement
review
v7.0.0
:Security/Security
|
Pinging @elastic/es-security |
jaymode
reviewed
Sep 25, 2018
| try { | ||
| return cache.computeIfAbsent(pattern, ignore -> buildAutomaton(pattern)); | ||
| } catch (ExecutionException e) { | ||
| return handleCacheException(e); |
There was a problem hiding this comment.
handleCacheException will always throw, so how about we have the return type of the method be a runtime exception and then this becomes throw unwrapCacheException(e) (I also renamed the method)
|
|
||
| `xpack.security.automata.cache.size`:: | ||
| The maximum number of items to retain in the automata cache. | ||
| Defaults to `500`. |
There was a problem hiding this comment.
500 seems low to me given the places we use automata. Any number we pick will be somewhat arbitrary but my gut says 10,000. Also, if we are concerned about excess memory usage, the lucene automata implement Accountable which provides ramBytesUsed so we can also limit by amount of memory/percent of heap in the cache.
- Refactor exception handling for cache - Change default cache size to 10,000 (was 500)
|
@jaymode I have addressed your feedback. |
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Oct 5, 2018
* master: Rename CCR stats implementation (elastic#34300) Add max_children limit to nested sort (elastic#33587) MINOR: Remove Dead Code from Netty4Transport (elastic#34134) Rename clsuterformation -> testclusters (elastic#34299) [Build] make sure there are no duplicate classes in third party audit (elastic#34213) BWC Build: Read CI properties to determine java version (elastic#34295) [DOCS] Fix typo and add [float] Allow User/Password realms to disable authc (elastic#34033) Enable security automaton caching (elastic#34028) Preserve thread context during authentication. (elastic#34290) [ML] Allow asynchronous job deletion (elastic#34058)
jasontedor
added a commit
to martijnvg/elasticsearch
that referenced
this pull request
Oct 5, 2018
* master: (63 commits) [Build] randomizedtesting: Allow property values to be closures (elastic#34319) Feature/hlrc ml docs cleanup (elastic#34316) Docs: DRY up CRUD docs (elastic#34203) Minor corrections in geo-queries.asciidoc (elastic#34314) [DOCS] Remove beta label from normalizers (elastic#34326) Adjust size of BigArrays in circuit breaker test Adapt bwc version after backport Follow stats structure (elastic#34301) Rename CCR stats implementation (elastic#34300) Add max_children limit to nested sort (elastic#33587) MINOR: Remove Dead Code from Netty4Transport (elastic#34134) Rename clsuterformation -> testclusters (elastic#34299) [Build] make sure there are no duplicate classes in third party audit (elastic#34213) BWC Build: Read CI properties to determine java version (elastic#34295) [DOCS] Fix typo and add [float] Allow User/Password realms to disable authc (elastic#34033) Enable security automaton caching (elastic#34028) Preserve thread context during authentication. (elastic#34290) [ML] Allow asynchronous job deletion (elastic#34058) HLRC: ML Adding get datafeed stats API (elastic#34271) ...
tvernum
added a commit
that referenced
this pull request
Oct 12, 2018
Building automatons can be costly. For the most part we cache things that use automatons so the cost is limited. However: - We don't (currently) do that everywhere (e.g. we don't cache role mappings) - It is sometimes necessary to clear some of those caches which can cause significant CPU overhead and processing delays. This commit introduces a new cache in the Automatons class to avoid unnecesarily recomputing automatons.
kcm
pushed a commit
that referenced
this pull request
Oct 30, 2018
Building automatons can be costly. For the most part we cache things that use automatons so the cost is limited. However: - We don't (currently) do that everywhere (e.g. we don't cache role mappings) - It is sometimes necessary to clear some of those caches which can cause significant CPU overhead and processing delays. This commit introduces a new cache in the Automatons class to avoid unnecesarily recomputing automatons.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Building automatons can be costly. For the most part we cache things
that use automatons so the cost is limited.
However:
mappings)
cause significant CPU overhead and processing delays.
This commit introduces a new cache in the security Automatons class
to avoid unnecesarily recomputing automatons.