Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache an exception result when retrieving config values #19

Merged
merged 3 commits into from
Jul 31, 2020

Conversation

bbaldino
Copy link
Member

ConfigValueSupplier caches the result of the retrieval so we don't re-run the retrieval code on every access, but if the ConfigValueSuppler#doGet chain throws, we bubble the exception up: a ConfigDelegate propagates the exception all the way up and an OptionalConfigDelegate catches it and returns null. This behavior, though, breaks the caching in ConfigValueSupplier as lazy doesn't cache the exception.

This PR adds a ConfigResult class (which is a very simplified version of kotlin's Result, which still has issues) in which we can store the result of retrieval--be it a value or an exception.

@codecov
Copy link

codecov bot commented Jul 30, 2020

Codecov Report

Merging #19 into master will decrease coverage by 0.44%.
The diff coverage is 63.63%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #19      +/-   ##
============================================
- Coverage     75.72%   75.27%   -0.45%     
- Complexity       42       43       +1     
============================================
  Files            15       16       +1     
  Lines           173      182       +9     
  Branches         13       14       +1     
============================================
+ Hits            131      137       +6     
- Misses           36       39       +3     
  Partials          6        6              
Impacted Files Coverage Δ Complexity Δ
...n/kotlin/org/jitsi/metaconfig/util/ConfigResult.kt 55.55% <55.55%> (ø) 0.00 <0.00> (?)
...g/jitsi/metaconfig/supplier/ConfigValueSupplier.kt 100.00% <100.00%> (ø) 2.00 <1.00> (ø)
...ain/kotlin/org/jitsi/metaconfig/SupplierBuilder.kt 100.00% <0.00%> (+6.25%) 8.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 a81dccf...a22fa54. Read the comment docs.

@bbaldino
Copy link
Member Author

I was confused why the coverage went down here (ConfigResult.kt is fully tested), but it looks like jacoco doesn't show inline functions as covered: jacoco/jacoco#654

@bbaldino bbaldino merged commit 8fa8f11 into jitsi:master Jul 31, 2020
@bbaldino bbaldino deleted the result branch July 31, 2020 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants