Replace Time.now with monotonic in lru cache and resource update #443
Conversation
miry
force-pushed
the
monotonic-resource-updated-at
branch
from
67abc55 to
17d3054
Compare
miry
force-pushed
the
monotonic-resource-updated-at
branch
2 times, most recently
from
b7fcb45 to
297eeab
Compare
miry
force-pushed
the
monotonic-resource-updated-at
branch
2 times, most recently
from
5d2afa4 to
4ce482b
Compare
miry
requested review from
casperisfine,
a team,
muz,
chopify and
atraver-shopify
and removed request for
a team
test/lru_hash_test.rb
Outdated
|
|
||
| Timecop.travel(2000) do | ||
| @lru_hash.set("d", create_circuit_breaker("d")) | ||
| sleep(3) |
There was a problem hiding this comment.
Uh, no sleep in test please. The suite is slow enough already.
You can use mocha to stub Process.clock_gettime. You can even write a small test helper that would work like Timecop.travel, would only take 5 lines of code.
There was a problem hiding this comment.
I wrote small mock solution for travel. it would require either both Time.now and Monotonic time mocking or merge changes from #441 to single PR -> remove any Time.now usage from semian.
There was a problem hiding this comment.
Introduced Time helper to freeze time with +/-duration.
Because circuit breaker requires Time.now for now and LRU hash uses Monotnic did universal stub mechanism for both clocks:
https://github.com/Shopify/semian/pull/443/files#diff-4063630c1592331c02bf7c3561f5eb835eaff847cc9d3bd993ea2488e3492cb3R3
|
Could you explain, for my learning, the value of this change?. NVM, I just read this |
miry
force-pushed
the
monotonic-resource-updated-at
branch
3 times, most recently
from
19b1f15 to
b888976
Compare
Resource updated_at is not require be timestamp value. Replaced the value with monotonic time, instead. It requires to update tests - timecop does not support monotonic time changes, replaced timecop with `sleep`. Intorduce TimeHelper to test monotonic.
miry
force-pushed
the
monotonic-resource-updated-at
branch
from
b888976 to
5ca75c0
Compare
Resource updated_at used only for LRU cache to clean unused resources.
There is no need in keeping timestamp.
Sidecar:
Implement basic travel time helper to cover Monotonic clocks.