Use value object for topology caching #2989
Closed
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
We now use a value object for caching the topology to avoid races in updating the cache timestamp. Also, we set the cache timestamp after obtaining the topology to avoid that I/O latency expires the topology cache. Closes #2986
Comment on lines
+844
to
+845
| if (shouldUseCachedValue(topology)) { | ||
| return topology; |
There was a problem hiding this comment.
the previous shouldUseCachedValue() would no longer be called in the patch targeting 3.3.4 so I was wondering if it makes senes to keep the arrangement stable for the SR and changing it for the 3.4 version.
For 3.3.4 we could have
protected boolean shouldUseCachedValue() {
return shouldUseCachedValue(cached);
}and call the old method.
There was a problem hiding this comment.
Feel free to apply these changes in the merge.
| * @since 3.3.4 | ||
| */ | ||
| protected boolean shouldUseCachedValue(@Nullable JedisClusterTopology topology) { | ||
| return topology != null && topology.getTime() + cacheTimeMs > System.currentTimeMillis(); |
There was a problem hiding this comment.
we could move the cacheTimeMs to the JedisClusterTopology having a package private method maxTime that does the calculation.
christophstrobl
added a commit
that referenced
this pull request
Sep 11, 2024
Move maxTime calculation for topology refresh to and delegate shouldUseCachedValue() to newly introduced overload. Original Pull Request: #2989
christophstrobl
added a commit
that referenced
this pull request
Sep 11, 2024
Move maxTime calculation for topology refresh to and delegate shouldUseCachedValue() to newly introduced overload. Retain application flow as much as possible. Original Pull Request: #2989
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.
We now use a value object for caching the topology to avoid races in updating the cache timestamp.
Also, we set the cache timestamp after obtaining the topology to avoid that I/O latency expires the topology cache.
Closes #2986