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

Option for caching in HTTP client #121

Closed
c0nstantx opened this issue Jan 1, 2020 · 10 comments
Closed

Option for caching in HTTP client #121

c0nstantx opened this issue Jan 1, 2020 · 10 comments

Comments

@c0nstantx
Copy link

Is your feature request related to a problem? Please describe

We use Patron's HTTP client to make calls to other APIs. There is the need to cache requests in order to reduce load in other services that aren't critical to get real time data.

Is this a bug? Please provide steps to reproduce, a failing test etc.

It's not a bug

Describe the solution

We should implement an optional HTTP caching layer in Patron's client based on HTTP Caching RFC 7234

Additional context

@c0nstantx c0nstantx mentioned this issue Jan 1, 2020
@mantzas
Copy link

mantzas commented Jan 1, 2020

@c0nstantx hi, if we want only to add caching at the client level this can be the responsibility of the application and does not need to be added to Patron (not making Patron bigger than it has to be). Server-side caching, on the other hand, could be an interesting and very difficult thing to do since HTTP has the notion of caching in various ways...

@c0nstantx
Copy link
Author

@mantzas what you say brings more work to each application, since we use Patron's HTTP client for other features also, like circuit breaker and tracing. With that in mind, we will have to implement them in each application too.
On the other hand, we could extract Patron's HTTP client to its own package and use it whenever is needed and decouple it from Patron. What do you think about that?

@mantzas
Copy link

mantzas commented Jan 1, 2020

@c0nstantx TBH I have seen only server-side caching implemented because this benefits actually all of the clients without the need of implementing something like this. Furthermore, since the server has the knowledge of the data that it serves, the server knows better when data is valid or invalid in order to be removed from the cache. I would consider implementing the server-side caching in order to solve this.

@c0nstantx
Copy link
Author

@mantzas What you are saying is a different topic of discussion.
What I want to do is caching in client side. It's like the behavior of a web browser but in REST calls from every app that uses Patron's client. There is the need to cache API responses from other services to reduce the number of calls made and improve performance.
HTTP caching is a known practice for not critical and/or real-time data and there is an RFC about that.
That is something that should become a part of Patron or at least as a separate HTTP client package, because there is a need for that functionality in many projects.

@mantzas
Copy link

mantzas commented Jan 1, 2020

There is a need for caching and I agree with that 100% but on the server-side. The RFC is exactly about implementing caching via HTTP 1.1 protocol on the server and the communication of this via the headers associated with that.

@c0nstantx
Copy link
Author

@mantzas this protocol isn't about implementing caching server side, but how cached data are communicated in the HTTP protocol and how they are requested and handled by clients. The ticket I opened is the part of the client that must cache responses whenever is needed.

@mantzas
Copy link

mantzas commented Jan 2, 2020

@c0nstantx doing some research there are some clients that include optionally caching but they are following what the server tells them.
Example: The server returns a response and set's the header to cache the result for 10min, the client cache will follow this instruction in order to cache the result.
The below link shows a full implementation of a caching mechanism that follows the rules of the server. The server is in control of what is cacheable and the client just blindly follows the instructions there.
Check out here:https://hc.apache.org/httpcomponents-client-4.5.x/tutorial/html/caching.html

We discussed it with other curators and it seems to be the proper way to move forward with this. This means that we have to implement 3 things:

  • a general caching package that can be used by others also (eta service, REMS already use something like that so we benefit all from it)
  • The optional server-side caching mechanism that caches the results also for clients that do not have caching capabilities implemented
  • The optional client-side caching that follows the caching instructions of the server response headers

We will open the above 3 tickets for implementing this mechanism so that the server has control over what is cacheable or not and the client following these instructions.
What do you think?

@c0nstantx
Copy link
Author

The reason that I wanted to enhance our own client instead of using existing implementations that support caching is that we already have circuit breaker and tracing support. Also, a feature that we want and the protocol doesn't include (because it's out of basic scope for HTTP caching) is force caching for a specific time limit.

When you say "caching package" what do you mean? An HTTP client with caching capabilities as a standalone package? If that is what you mean, we could also extract all existing client logic (as mentioned in previous comment) we already have in Patron.

The server side caching and proper response headers build, surely, is nice but still it can't help clients that don't support caching, since they will ignore the headers and always call to service, creating network traffic.

The last thing you mention is what I started working in this issue (in Patron's HTTP client), but also with adding specific caching TTL. That was because there are cases that we want to cache responses for a specific time no matter if the server supports caching or not.

So, in general, I agree with these points.

@mantzas
Copy link

mantzas commented Jan 2, 2020

We have opened the tickets mentioned above:

the last two depend on the first one and some other open issues...

@c0nstantx
Copy link
Author

I am closing that issue, since the new ones are covering what it is described here.

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

No branches or pull requests

2 participants