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

New feature: local cache #35

Merged
merged 21 commits into from Sep 3, 2018
Merged

New feature: local cache #35

merged 21 commits into from Sep 3, 2018

Conversation

icyleaf
Copy link
Owner

@icyleaf icyleaf commented Aug 30, 2018

Related to #24

@ghost ghost assigned icyleaf Aug 30, 2018
@ghost ghost added the in progress label Aug 30, 2018
@icyleaf
Copy link
Owner Author

icyleaf commented Aug 30, 2018

Update a little test:

Complie source code and get request "http://httpbin.ogr/anything"

$ time crystal main.cr
crystal main.cr  6.79s user 1.21s system 197% cpu 4.039 total

$ time crystal main.cr
crystal main.cr  2.18s user 0.50s system 133% cpu 2.006 total

It speed up 3x fast!

Copy link

@j8r j8r left a comment

Choose a reason for hiding this comment

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

That's a good feature, this also needs specs.
I find Halite::Features and Halite::Feature are quite confusing.
I think having directly Halite::Logger and Halite::Cache would clearer.

# r.headers # => {..., "X-Cached-At" => "2018-08-30 10:41:14 UTC", "X-Cached-By" => "Halite", "X-Cached-Expires-At" => "2018-08-30 10:41:19 UTC", "X-Cached-Key" => "2bb155e6c8c47627da3d91834eb4249a"}}
# ```
class Cache < Feature
DEFAULT_PATH = "cache/"
Copy link

@j8r j8r Aug 30, 2018

Choose a reason for hiding this comment

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

It's not a good idea to create a directory in the root directory, at least create in /tmp
Maybe a better approach would be:

  1. On a new Cache object creation, a temporary directory will be created with https://crystal-lang.org/api/master/Tempfile.html (Tempfile.new("halite-cache").path)
  2. Add a finalize method that will delete the directory when the Object is destructed (#delete)

Note: the API will likely change in the next release crystal-lang/crystal#6485

Copy link
Owner Author

Choose a reason for hiding this comment

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

This commit just a simple test, it was not following my spec. May be push it early.

Copy link

@j8r j8r left a comment

Choose a reason for hiding this comment

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

Tempfile can be used for the cache

@icyleaf
Copy link
Owner Author

icyleaf commented Aug 31, 2018

I find Halite::Features and Halite::Feature are quite confusing.
I think having directly Halite::Logger and Halite::Cache would clearer.

That's my problem, i did not design the picture enough, I'm working on it, i will removing Features and let feature under Halite module.

It will a new PR to do (#36)

…ture under Halite and some method changes.

Feature:

- `Halite::Features::Logger` to `Halite::Logger`
- `Halite::Features.register` to `Halite.register_feature`
- `Halite::Features[]/[]?` to `Halite.features/features?`
- `Halite::Features.avaiables` to `Halite.has_feature?`
- `Halite::Feature::Interceptor::Chain` to `Halite::Feature::Chain`
…d merge Halite::MimeTypes.register_adapter/register_alias into Halite::MimeType.register
…d merge Halite::MimeTypes.register_adapter/register_alias into Halite::MimeType.register
@icyleaf
Copy link
Owner Author

icyleaf commented Aug 31, 2018

@j8r I was updated new design about local cache, please review the top content at #24. With new design, it does not use Tempfile.

Copy link
Owner Author

@icyleaf icyleaf left a comment

Choose a reason for hiding this comment

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

Ready to merge 🎉

@icyleaf icyleaf merged commit b36866b into master Sep 3, 2018
@icyleaf icyleaf deleted the feature/local-cache branch September 3, 2018 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants