Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Fix casting issue with cache expiration #215

Merged

Conversation

jmazanec15
Copy link
Member

Issue #, if available:
#203

Description of changes:
Previously, we did not cast KNNSettings.state().getSettingValue(KNNSettings.KNN_CACHE_ITEM_EXPIRY_TIME_MINUTES)) to TimeValue and then get the long value. This was causing the cache to never be rebuilt, causing the circuit breaker limit to exceed 100%. This change fixes that and adds a test to validate it.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jmazanec15 jmazanec15 added the Bug Fixes Change that fixes a bug label Sep 4, 2020
@codecov
Copy link

codecov bot commented Sep 4, 2020

Codecov Report

Merging #215 into master will increase coverage by 0.27%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #215      +/-   ##
============================================
+ Coverage     77.34%   77.61%   +0.27%     
- Complexity      307      308       +1     
============================================
  Files            50       50              
  Lines          1192     1193       +1     
  Branches        101      101              
============================================
+ Hits            922      926       +4     
+ Misses          221      219       -2     
+ Partials         49       48       -1     
Impacted Files Coverage Δ Complexity Δ
...istroforelasticsearch/knn/index/KNNIndexCache.java 92.30% <100.00%> (+2.98%) 35.00 <0.00> (+1.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3d2816...192943d. Read the comment docs.

@jmazanec15 jmazanec15 changed the title fix casting issue with cache expiration Fix casting issue with cache expiration Sep 4, 2020
Copy link
Member

@vamshin vamshin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. LGTM!

@jmazanec15 jmazanec15 merged commit 66363b1 into opendistro-for-elasticsearch:master Sep 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Fixes Change that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants