Look into CachingHttpClient #129

Closed
dakrone opened this Issue Apr 4, 2013 · 8 comments

Comments

Projects
None yet
2 participants
@dakrone
Owner

dakrone commented Apr 4, 2013

This would require using a single DefaultHttpClient for multiple requests, which clj-http currently does NOT do.

Implementing this means implementing a way to re-use the HttpClient for multiple requests.

@dcj

This comment has been minimized.

Show comment Hide comment
@dcj

dcj Apr 6, 2013

Lee,
I am just finishing up a caching wrapper for clj-http.
We looked (very briefly) at CachingHttpClient, and decided on another approach....
At present, we wrap core/request with our caching wrapper, using robert.hooke.
We use core.cache, and our first implementation uses mongodb via monger's implementation of core.cache & mongo. The initial version seems to work well. Hooking core/request is not optimal for me in general, I'd like another way to insert my cache wrapper between client/request and core/request. If you'd like to discuss this, please get in contact....

dcj commented Apr 6, 2013

Lee,
I am just finishing up a caching wrapper for clj-http.
We looked (very briefly) at CachingHttpClient, and decided on another approach....
At present, we wrap core/request with our caching wrapper, using robert.hooke.
We use core.cache, and our first implementation uses mongodb via monger's implementation of core.cache & mongo. The initial version seems to work well. Hooking core/request is not optimal for me in general, I'd like another way to insert my cache wrapper between client/request and core/request. If you'd like to discuss this, please get in contact....

@dakrone

This comment has been minimized.

Show comment Hide comment
@dakrone

dakrone Apr 8, 2013

Owner

Here's how I'd recommend setting up caching for clj-http currently:

(with-middleware #{wrap-cache-requests ;; <-- your caching middleware
                   wrap-request-timing
                   wrap-lower-case-headers
                   wrap-query-params
                   wrap-basic-auth
                   wrap-oauth
                   wrap-user-info
                   wrap-url
                   wrap-redirects
                   wrap-decompression
                   wrap-input-coercion
                   wrap-additional-header-parsing
                   wrap-output-coercion
                   wrap-exceptions
                   wrap-accept
                   wrap-accept-encoding
                   wrap-content-type
                   wrap-form-params
                   wrap-nested-params
                   wrap-method
                   wrap-cookies
                   wrap-links
                   wrap-unknown-host}
  (get "http://www.example.com"))

Where you could put your caching middleware anywhere in the list. Another alternative is:

(with-middleware (concat [wrap-caching-middleware] all-middleware)
  (get "http://www.example.com"))
Owner

dakrone commented Apr 8, 2013

Here's how I'd recommend setting up caching for clj-http currently:

(with-middleware #{wrap-cache-requests ;; <-- your caching middleware
                   wrap-request-timing
                   wrap-lower-case-headers
                   wrap-query-params
                   wrap-basic-auth
                   wrap-oauth
                   wrap-user-info
                   wrap-url
                   wrap-redirects
                   wrap-decompression
                   wrap-input-coercion
                   wrap-additional-header-parsing
                   wrap-output-coercion
                   wrap-exceptions
                   wrap-accept
                   wrap-accept-encoding
                   wrap-content-type
                   wrap-form-params
                   wrap-nested-params
                   wrap-method
                   wrap-cookies
                   wrap-links
                   wrap-unknown-host}
  (get "http://www.example.com"))

Where you could put your caching middleware anywhere in the list. Another alternative is:

(with-middleware (concat [wrap-caching-middleware] all-middleware)
  (get "http://www.example.com"))
@dcj

This comment has been minimized.

Show comment Hide comment
@dcj

dcj Apr 8, 2013

Lee,

Thanks for your response and suggestions.

Regarding your second alternative, does "(with-middleware all-middleware" produce
exactly the same middleware in exactly the same order as wrap-request does today?
If not, I'm concerned about subtle behavior differences....

My concern about the first alternative is that I will need to keep track of your current middleware stack/ordering
from release to release, which I'd rather avoid, if possible

Given that the middleware I would like to add is "right before" core/request, what about this:

(def cached-request
  (wrap-request (wrap-cache-requests #'core/request)))

And then I can use the resulting cached-request to build convenience functions like cached-get?

I think this will give me everything I need, and without hooke-ing core/request, which affects
all callers of core/request, which may not be what is desired in all cases....

dcj commented Apr 8, 2013

Lee,

Thanks for your response and suggestions.

Regarding your second alternative, does "(with-middleware all-middleware" produce
exactly the same middleware in exactly the same order as wrap-request does today?
If not, I'm concerned about subtle behavior differences....

My concern about the first alternative is that I will need to keep track of your current middleware stack/ordering
from release to release, which I'd rather avoid, if possible

Given that the middleware I would like to add is "right before" core/request, what about this:

(def cached-request
  (wrap-request (wrap-cache-requests #'core/request)))

And then I can use the resulting cached-request to build convenience functions like cached-get?

I think this will give me everything I need, and without hooke-ing core/request, which affects
all callers of core/request, which may not be what is desired in all cases....

@dakrone

This comment has been minimized.

Show comment Hide comment
@dakrone

dakrone Apr 8, 2013

Owner

Regarding your second alternative, does "(with-middleware all-middleware" produce exactly the same middleware in exactly the same order as wrap-request does today?

No, that's an unfortunate downside. I plan to refactor the default middleware list into a list for other people to use also. It's correct to be concerned about subtle behavior differences, so I'll fix that.

Given that the middleware I would like to add is "right before" core/request, what about this: ...(elided)

That seems like a reasonable thing to do, then declaring your own get, post, put, etc functions is fairly trivial.

Owner

dakrone commented Apr 8, 2013

Regarding your second alternative, does "(with-middleware all-middleware" produce exactly the same middleware in exactly the same order as wrap-request does today?

No, that's an unfortunate downside. I plan to refactor the default middleware list into a list for other people to use also. It's correct to be concerned about subtle behavior differences, so I'll fix that.

Given that the middleware I would like to add is "right before" core/request, what about this: ...(elided)

That seems like a reasonable thing to do, then declaring your own get, post, put, etc functions is fairly trivial.

@dakrone

This comment has been minimized.

Show comment Hide comment
@dakrone

dakrone Apr 8, 2013

Owner

@dcj with that latest commit, you would be able to do:

(with-middleware (conj default-middleware my-caching-middleware)
  (get ...)
  (post ...))
Owner

dakrone commented Apr 8, 2013

@dcj with that latest commit, you would be able to do:

(with-middleware (conj default-middleware my-caching-middleware)
  (get ...)
  (post ...))
@dcj

This comment has been minimized.

Show comment Hide comment
@dcj

dcj Apr 9, 2013

@dakrone That is a great change! I was thinking about something along these lines earlier today, and was intending to code it up, but my initial ideas weren't as good as yours, and then I came up with the approach I outlined above to avoid having to change cli-http. Providing the exact vector of default middleware is very helpful, it will be make integration by others a lot easier.

dcj commented Apr 9, 2013

@dakrone That is a great change! I was thinking about something along these lines earlier today, and was intending to code it up, but my initial ideas weren't as good as yours, and then I came up with the approach I outlined above to avoid having to change cli-http. Providing the exact vector of default middleware is very helpful, it will be make integration by others a lot easier.

@dakrone

This comment has been minimized.

Show comment Hide comment
@dakrone

dakrone Apr 9, 2013

Owner

I've released 0.7.1 with this change if you want to give it a try and let me know how it works out.

Owner

dakrone commented Apr 9, 2013

I've released 0.7.1 with this change if you want to give it a try and let me know how it works out.

@dakrone

This comment has been minimized.

Show comment Hide comment
@dakrone

dakrone Dec 7, 2013

Owner

Closing, as I think any caching should be implemented as a middleware rather than the Apache Caching Client.

Owner

dakrone commented Dec 7, 2013

Closing, as I think any caching should be implemented as a middleware rather than the Apache Caching Client.

@dakrone dakrone closed this Dec 7, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment