-
Notifications
You must be signed in to change notification settings - Fork 1
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
Basic TTL engine #4
base: master
Are you sure you want to change the base?
Conversation
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you multiply with the 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
What's it about?
Implementing TTL engine.