fix(redis): mitigate JedisPool depletion #419
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
Fixes redis interactions in RedisPermissionRepository that can result in depleting the redis pool. Specifically: * converts smembers calls to sscan calls, and gets/returns an object from the pool to do so. * removes nested calls to redisClientDelegate that were holding on to pool objects * partitions lookup of users into smaller batches * partitions role lookup Additionally, uses InstrumentedJedisPool to enable metrics collection on redis interactions.
robzienert
reviewed
Jun 6, 2019
| return new InstrumentedJedisPool( | ||
| registry, | ||
| new JedisPool(redisPoolConfig, host, port, timeout, password, database, null), | ||
| "fiat"); |
There was a problem hiding this comment.
I made this generic (I should deprecate InstrumentedJedisPool): https://github.com/spinnaker/kork/blob/master/kork-telemetry/src/main/java/com/netflix/spinnaker/kork/telemetry/InstrumentedProxy.java
There was a problem hiding this comment.
pending spinnaker/kork#313
There was a problem hiding this comment.
actually scratch that, InstrumentedProxy doesn't work here (it would just instrument the pool methods not the pooled object itself)
| }); | ||
| } catch (Exception e) { | ||
| log.error("Storage exception reading " + id + " entry.", e); | ||
| } | ||
| } | ||
|
|
||
| private Set<String> scanSet(String key) { | ||
| final Set<String> results = new HashSet<>(); | ||
| final AtomicReference<String> cursor = new AtomicReference<>(ScanParams.SCAN_POINTER_START); |
clanesf
pushed a commit
to clanesf/fiat
that referenced
this pull request
Jul 12, 2019
* chore(dependencies): gradle 5.4.1 * fix(redis): mitigate JedisPool depletion Fixes redis interactions in RedisPermissionRepository that can result in depleting the redis pool. Specifically: * converts smembers calls to sscan calls, and gets/returns an object from the pool to do so. * removes nested calls to redisClientDelegate that were holding on to pool objects * partitions lookup of users into smaller batches * partitions role lookup Additionally, uses InstrumentedJedisPool to enable metrics collection on redis interactions.
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.
Fixes redis interactions in RedisPermissionRepository that can result in
depleting the redis pool.
Specifically:
from the pool to do so.
to pool objects
Additionally, uses InstrumentedJedisPool to enable metrics collection
on redis interactions.