Conversation
benhurdavies
changed the title
benhurdavies
added
Ready for Review
TarSzator
left a comment
TarSzator
left a comment
There was a problem hiding this comment.
I did not review everything yet. Found to much stuff :-) Sorry.
| Whenever a get method called it check the element exist and check it is expired or not. If it is expired then removes the item and clean previous buckets. | ||
| There is also a `runGC()` method in ttl cache. It will clean the buckets in between last cleaned time and now. | ||
| TTL engine do not run `runGC()` method automatically or in an interval. | ||
| We do not need to iterate or look all items for cleaning because of expired time partitioning. check below image for more information of architecture. |
There was a problem hiding this comment.
This sentence is misleading.
I would change it to:
Due to the time partitioning we do not need to iterate over all items for garbage collection which adds performance to the process. Check below image for more information of architecture.
|
|
||
| let lowestTimePartition = Date.now(); | ||
|
|
||
| this.add = (key, value, ttl = defaultTTL) => { |
There was a problem hiding this comment.
Just something we might want to consider. When we remove this ttl and only have the default ttl we have more performance optimization potential.
| const payload = { value, ttl: expireTTL, tNode }; | ||
| store[hashTableProp.add](key, payload); |
There was a problem hiding this comment.
Why buffe
store[hashTableProp.add](key, { value, ttl: expireTTL, tNode });
| if (payload) { | ||
| const { ttl, value } = payload; | ||
| if (checkIfElementExpire({ ttl, key })) return undefined; | ||
| else return value; | ||
| } | ||
| return undefined; |
There was a problem hiding this comment.
if (!payload) {
return undefined;
}
const { ttl, value } = payload;
return checkIfElementExpire({ ttl, key }) ? undefined : value;
| this.has = key => { | ||
| return ( | ||
| store[hashTableProp.has](key) && | ||
| !checkIfElementExpire({ ttl: store[hashTableProp.get](key), key }) |
There was a problem hiding this comment.
I believe there is a bug in you test ;-)
!checkIfElementExpire({ ttl: store[hashTableProp.get](key), key }) needs to be
!checkIfElementExpire({ ttl: store[hashTableProp.get](key).ttl, key }) right?
|
|
||
| function getBackwardTimeIndex({ time, interval }) { | ||
| const timeParts = (time / interval) | 0; | ||
| return timeParts * interval; |
There was a problem hiding this comment.
Why do you multiply with the interval?
There was a problem hiding this comment.
for better debugging.
| // interval : milliseconds (better to be factors of 60 (minutes)) | ||
| function getForwardTimeIndex({ time, interval }) { | ||
| const timeParts = (time / interval) | 0; | ||
| return timeParts * interval + interval; |
There was a problem hiding this comment.
Why multiply with the interval? why not just timeParts + 1?
| const list = new DoublyLinkedList(); | ||
| timePartition[hashTableProp.add](timeIndex, list); |
There was a problem hiding this comment.
const list is actually the timePartition and what you call timePartition is either the partitionTable or timePartitions
| cleanNotExpiredBucket(nextCleanBucket); | ||
| }; | ||
|
|
||
| function getTimeBucket(expireTTL) { |
There was a problem hiding this comment.
In the context of partitions this function should be called getTimePartition right?
| }; | ||
|
|
||
| this.runGC = () => { | ||
| const cleanTo = getBackwardTimeIndex({ time: Date.now(), interval: timeIndexInterval }); |
There was a problem hiding this comment.
Just realize maybe not for this version but for future version
a) Providing interval all the time is not very functional programmy. We need more like a createIntervalHandler function that gets the defaultTTL and calculates the best index and then returns an object with methods getPreviousTimeIndex and getCurrentTimeIndex based only on time input
b) The whole timepartition thing is kind of a data structure ;-)
|
@benhurdavies I think this needs a refactorying as well after TSify merge |
2 participants
What's it about?
Implementing TTL engine.