-
Notifications
You must be signed in to change notification settings - Fork 594
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 refresh without remove #2385 #2522
Conversation
Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged. For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask! |
OK TO TEST |
Test PASSed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some race conditions here but since it's a cache perhaps that's no problem.
The logic looks OK to me, but can perhaps be simplified, and might possibly be faster as well, by avoiding store.synchronous()
?
akka-http-caching/src/main/scala/akka/http/caching/LfuCache.scala
Outdated
Show resolved
Hide resolved
akka-http-caching/src/main/scala/akka/http/caching/LfuCache.scala
Outdated
Show resolved
Hide resolved
akka-http-caching/src/main/scala/akka/http/caching/LfuCache.scala
Outdated
Show resolved
Hide resolved
… use of promise (#2385)
Test PASSed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement, one more thought
akka-http-caching/src/main/scala/akka/http/caching/LfuCache.scala
Outdated
Show resolved
Hide resolved
Test FAILed. |
Why does Jenkins PR Validation fails saying sed: can't read ./akka-http2-support/target/test-reports/h2spec-junit.xml: No such file or directory I haven't changed anything related to that |
PLS BUILD |
Test PASSed. |
Test PASSed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
akka-http-caching/src/main/scala/akka/http/caching/scaladsl/Cache.scala
Outdated
Show resolved
Hide resolved
akka-http-caching/src/main/scala/akka/http/caching/scaladsl/Cache.scala
Outdated
Show resolved
Hide resolved
akka-http-caching/src/test/scala/akka/http/caching/ExpiringLfuCacheSpec.scala
Outdated
Show resolved
Hide resolved
Test PASSed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ashishkujoy. The feature seems useful, a few detail questions and suggestions below.
akka-http-caching/src/main/scala/akka/http/caching/scaladsl/Cache.scala
Outdated
Show resolved
Hide resolved
akka-http-caching/src/main/scala/akka/http/caching/scaladsl/Cache.scala
Outdated
Show resolved
Hide resolved
Test PASSed. |
Test PASSed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks a lot @ashishkujoy!
Great! |
Purpose
Provided a put method in cache API and implemented the same in Lfu cache, this method takes a key and future value, if the key is not already cached it save key into the cache and in case key is already cached, it replaces the old cache value on once future value successfully completes
References
#2385
References #xxxx
Changes
Added put method in Cache
Background Context