minotar / imgd

Minotar is a global avatar service that pulls your head off of your Minecraft.net skin, and allows it for use on several thousand sites.
https://minotar.net/
The Unlicense
203 stars 62 forks source link

Refactor caching and add UUID support #189

Closed LukeHandle closed 3 years ago

LukeHandle commented 6 years ago

It's been in testing on minotar.net for the last week or so now. Only issues seen are with the TTL for Skins are the use of the "lru" cache (not removing from the expiry list).

Needs fix for expiry list not being pruned on lru eviction

connor4312 commented 6 years ago

wow some of this is some ooold code. I'll give it some 👀 when I get home tonight.

LukeHandle commented 6 years ago

Oh no, you'll see some of the lazy stuff I did last night with adding the UUID routes 🙊

For context, I'm on holiday for a week, and really wanted to get the UUID support officially out the door before I headed off.

Seeing as you're around... Have you got a UUID we can put in this box: https://github.com/minotar/minotar.net/pull/14/files#diff-eacf331f0ffc35d4b482f1d15a887d3bR57 :D

There is definitely outstanding work with the storage package (I plan to implement the underlying lru package vs. the current wrapper we use). https://github.com/bluele/gcache looks interesting, worth a test. File based backend for skins is probably more sensible as well, but then expiration/management of that is "fun".

Also, splitting the storage into a separate repo might be sensible. Realistically, only the Redis is "stable" currently.

Had to make it not test/build in the storage package, but passes: wercker status

lenovouser commented 6 years ago

@LukeHandle is this the version that is currently running on minotar?

LukeHandle commented 6 years ago

@lenovouser It mostly is yes (or maybe entirely is). I do remember encountering some bugs with the in-memory LRU caching (leaking memory as it evicts items/not removing them from the expiry list), but using a lower TTL / not using that engine resolved that issue. The fix shouldn't be that bad either, but the fire needed putting out... In which case, it probably is entirely live, though not using the other Storage types.

I guess I didn't feel like merging until then 😬 I suspect I'll re-visit this within the next 6 months, but while the frontend works, it's easier to prioritise everything else in my life. Certain programming bits at work popped up and have been fun 🤷‍♂️

For reference, https://github.com/minotar/imgd/issues/181 tracked the issue / rollout. I had to add the Profiling option due to the memory issue...

LukeHandle commented 6 years ago

@lenovouser Were you just intrigued, or something awry?

lenovouser commented 6 years ago

@LukeHandle Thanks - no, nothing awry. Just intrigued if it is good to go when being self hosted. Was using Redis already before so there should be no issues I guess.

LukeHandle commented 6 years ago

@lenovouser it'll take a little warming/ramp-up if using Usernames, but that's life these days. I'd encourage enabling Redis "save" (persistence) for the Username -> UUID cache as that data (especially 404 users) is hugely valuable for 30+ days. That'll be the most pain for ratelimits if Redis restarts.

Let me know if I can help out / you spot any bugs!

lenovouser commented 6 years ago

@LukeHandle ah, thanks. Will do!

LukeHandle commented 3 years ago

FWIW, using lru is still "okay" for shorter TTL caching. The same point still stands though... skins should likely be saved to file.

In prod, both UUID and UserData is sent to shared Redis', there's a little variation in where the Skin's themselves are stored (Redis vs. local).

Since added Redis Cluster support as a quickfix to allow migration to Redis cluster.

At this stage, this code has been live for years already. Redis remains stable. Memory/LRU isn't recommended for UUID or UserData.

Seems little point in fixing the GH / Travis / Wercker checks/tests when the entire codebase needs revamping.