-
Notifications
You must be signed in to change notification settings - Fork 52
Implement UpdateTimeToLive [HZ-5249][API-1789]
#997
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
base: master
Are you sure you want to change the base?
Conversation
UpdateTimeToLiveUpdateTimeToLive [HZ-5249][API-1789]
…p-client into fix-update-timetolive
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #997 +/- ##
==========================================
+ Coverage 81.46% 81.51% +0.05%
==========================================
Files 1054 1054
Lines 25420 25427 +7
==========================================
+ Hits 20709 20728 +19
+ Misses 4711 4699 -12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| Assert.Less(elapsed, maxTTL * 1000); | ||
|
|
||
| // Close to min TTL | ||
| Assert.Less(elapsed, minTTL * 1000 * 2); |
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.
i am not sure if this will always work if the server is working slow, etc.
Does the ContainsKeyAsync trigger eviction check on expiry, if so, it may be safe but if we do it on scheduled basis on the server side, there may be a time diff until that scheduler is triggered and entry is evicted. If that is the case, this check may fail depending on the scheduler interval.
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.
Right, it may not work well in slow cases. Do you have any suggestion?
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.
remove that check :) By the way you normally do not need to introduce your own latch but we have assertTrueEventually kind of test utilities which would simply the test. Just check Java side. By the way, did we cover all the existing tests at the Java client for this API? If not, we can also add missing ones.
Implemented missing
Map.UpdateTimeToLiveAPI.