Skip to content
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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Basic TTL engine #4

wants to merge 14 commits into from

Conversation

benhurdavies
Copy link
Owner

@benhurdavies benhurdavies commented Jun 14, 2020

What's it about?

Implementing TTL engine.

  • Time partition with expired value.
  • Implement cache engine methods.

@benhurdavies benhurdavies added the WIP work in progress label Jun 14, 2020
@benhurdavies benhurdavies changed the title WIP: started ttl engine. implemented timeIndex method and test Basic TTL engine Jun 27, 2020
@benhurdavies benhurdavies added Ready for Review Ready for review and removed WIP work in progress labels Jun 27, 2020
Copy link
Collaborator

@TarSzator TarSzator left a 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.
Copy link
Collaborator

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) => {
Copy link
Collaborator

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.

Comment on lines +23 to +24
const payload = { value, ttl: expireTTL, tNode };
store[hashTableProp.add](key, payload);
Copy link
Collaborator

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 });

Comment on lines +29 to +34
if (payload) {
const { ttl, value } = payload;
if (checkIfElementExpire({ ttl, key })) return undefined;
else return value;
}
return undefined;
Copy link
Collaborator

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 })
Copy link
Collaborator

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;
Copy link
Collaborator

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?

Copy link
Owner Author

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;
Copy link
Collaborator

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?

Comment on lines +74 to +75
const list = new DoublyLinkedList();
timePartition[hashTableProp.add](timeIndex, list);
Copy link
Collaborator

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) {
Copy link
Collaborator

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 });
Copy link
Collaborator

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 benhurdavies linked an issue Aug 5, 2020 that may be closed by this pull request
@TarSzator TarSzator removed the Ready for Review Ready for review label May 12, 2024
@TarSzator
Copy link
Collaborator

@benhurdavies I think this needs a refactorying as well after TSify merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TTL engine
2 participants