Cache Linode API GET calls #113
Conversation
mhmxs
force-pushed
the
cache-linode-client
branch
from
18ed216 to
114d9ad
Compare
mhmxs
added
dependencies
mhmxs
force-pushed
the
cache-linode-client
branch
from
4b262c2 to
7265b5b
Compare
| @@ -5,6 +5,7 @@ go 1.21 | |||
| toolchain go1.21.5 | |||
|
|
|||
| require ( | |||
| github.com/bxcodec/httpcache v1.0.0-beta.3 | |||
There was a problem hiding this comment.
I'm not sure if this is a concern or not, but just noticed this package(and assuming the gotcha package as well) have been on the same beta version for over 3 years at this point with no updates, are we ok with adopting these dependencies? if we want to accept them I could buy into that, just want to make sure we are aware of them
There was a problem hiding this comment.
Can we use ETag caching here?
There was a problem hiding this comment.
@eljohnson92 Other libraries (what i found) are archived, so i found this one a better option than writing our own.
I'm open to use something else, do you have any preference?
There was a problem hiding this comment.
@srust Maybe i'm wrong, but Linode API doesn't send ETag header, at least the for web ui. Could you please clarify your suggestion?
There was a problem hiding this comment.
@mhmxs as an alternative to this, could we perhaps use a more general purpose go caching library to store cache instead of the purpose-built http caching library? I would be open to using this if we thing it would be the best option, just want to explore some alternatives as well
There was a problem hiding this comment.
@eljohnson92 This was the simplest to implement. The alternatives i found:
- stores
stringor[]byte, so we have to serialize data - stores
anyorinterface{}, so we have to type enforce type after every get - needs an extra layer between business logic and Llinode client
- already archived
- not designed for high concurrency
- doesn't support TTL
So i'm glad to use another library, if you have better option. This library was the best result of my researching :)
There was a problem hiding this comment.
Bummer about ETag - oh well.
| ) | ||
|
|
||
| var ( | ||
| initClient sync.Once |
There was a problem hiding this comment.
This change seems to limit us to using one API client (and thus one access token) for all CAPL-managed clusters. Is there another way to have both caching for the API while supporting something like per-cluster Linode tokens?
There was a problem hiding this comment.
@cbzzz we had one access token before this change, and unnecessary client creations (access token never changes). So i think my version is better than the previous one (more configurable, and less footprint). But your point is also valid, it needs a new concept how to define per-cluster Linode token. Once we change to per-cluster, this cache solution won't be the best for us. So this is an interesting question.
|
The upcoming change invalidates this one: #142 |
This change introduces a caching layer in front of the Linode client.
Fixes: #92