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

Added file driver for file based caching #34

Merged
merged 3 commits into from Nov 4, 2016
Merged

Conversation

BRIMIL01
Copy link
Contributor

No description provided.

@drewdeponte drewdeponte self-assigned this Nov 1, 2016
Copy link
Contributor

@drewdeponte drewdeponte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have few small things, and one questionable important thing.

def set(key, entity)
cache = { 'value' => entity.value }
if entity.expiration
expiration = Time.now.to_i + entity.expiration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't want to UTC this first?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figured since you were using it in the internal_timestamp probably would want to be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I fix it in the in_memory_driver as well?

return Hitnmiss::Driver::Miss.new if cached_entity.nil?

if cached_entity.has_key?('expiration')
if Time.now.to_i >= cached_entity['expiration']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the other expiration is UTC'd I would expect this one to be as well.

options[:fingerprint] = cached_entity['fingerprint']
end
if cached_entity.has_key?('updated_at')
options[:updated_at] = Time.parse(cached_entity['updated_at'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this handles TZ correctly if I remember correctly.

subject { Hitnmiss::FileDriver.new(folder) }

before do
subject
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you using the construction of the class to do the mkdir_p?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am

context 'when given entity has a last_modified' do
it 'caches the given value with key, expiration, and last_modified' do
entity = Hitnmiss::Entity.new('some_value', expiration: 1,
fingerprint: 'foofingerprint',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you intend for fingerprint to be set here? It looks like from the description you don't care.

context 'when given entity has a last_modified' do
it 'caches the given value with key, expiration, and last_modified' do
entity = Hitnmiss::Entity.new('some_value',
fingerprint: 'foofingerprint',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question about caring about fingerprint in this test case.

build_hit_keyword_args(cached_entity))
end

def all(keyspace)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we also need to take into consideration, expiration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the conventions in the in_memory_driver

end

describe '#all' do
it 'returns only the values whose keys begin with the keyspace' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this also needs to take into consideration if the entities are expired or not, and only return values for cached entities that haven't already expired.

@drewdeponte
Copy link
Contributor

👍

@BRIMIL01 BRIMIL01 merged commit c8ac96d into master Nov 4, 2016
@BRIMIL01 BRIMIL01 deleted the add_file_store_support branch November 4, 2016 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants