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

Refreshing avatars #349

Closed
wants to merge 3 commits into from
Closed

Conversation

henieek
Copy link
Contributor

@henieek henieek commented Apr 23, 2013

Hello,

This pull-request changes the way of storing avatars in the cache and fixes problem with non-refreshing avatars. Previous way was relying on userId/userEmail, so in case user changed his avatar on gravatar it had no effect in the app (it always used the cached version). Currently, avatar file is cached as Base64 string from the avatar url.
I'm wondering how to deal with old avatars. Storing all the avatars with some prefix (let's say: user_id__Base64) and deleting user_id__* before storing new one would work, but I like the idea of periodical service execution which will be maintaining cache state so it will never exceed rational limit - as the cache can contain also current avatars of the users, which are no longer active.
What's your opinion?

Best.

@kevinsawicki
Copy link
Contributor

You could store all avatars in a folder with the user id as the name and just delete all current files in that folder before writing a new one if the avatar doesn't already exist in that folder.

I like this though, refreshing the avatars would be good to add for the next release.

@atermenji atermenji closed this in 47042f0 Jul 25, 2013
@atermenji
Copy link
Contributor

Thanks for the fix, I've resolved conflicts and merged this into master.
Also I've added some code to store avatars for a specific user in a separate folder, like @kevinsawicki suggested.

The problem with refreshing avatars is not completely fixed with this patch.
Avatar url and gravatar_id returned by API remain the same when changing an avatar on gravatar.com.
And I don't know if this is a GitHub API issue or just an internal logic of Gravatar service.

@NobbZ
Copy link

NobbZ commented Jul 26, 2013

I don't think there is any need to cache Gravatar images it self. The
generated ID is simple an MD5 hash of the email address provided, this is
why the ID doesn't change. Also the URL is simple this hash.
Soi the preferred way to get the gravatar should be to just load it
directly.

Since MD5 ius such a simple algorithm I wouldn't even mind to cache the
generated hash for longer as a single request. I don't think that there
will be a measurable difference in serverload between fetching the ID from
database or generating it again for a request. But storing and providing
the cached gravatar itself will definitely increase your load!

@atermenji
Copy link
Contributor

@NobbZ do you propose to completely remove file system gravatar images cache?

@NobbZ
Copy link

NobbZ commented Jul 26, 2013

edit, original post @ the bottom!

Yikes! Just see that this is the github android issuetracker… When I was writing this, and the post before, I assumed for some reason that this would be about padrino… So I leave the post for public information, but change my mind in any other way!

Since we are talking about a mobile application, that probably uses an expensive dataplan, there should be extensive caching!

I wouldn't mind about old gravatars, as long as the app catches up at most a day late.

So I would suggest, just to cache the gravatars for about 24 hours and when they are aged, use HTTP-Header-Etag (http://en.wikipedia.org/wiki/HTTP_ETag) to check at gravatar.com if there is a new image for that given gravatar and then reset the timer and wait another 24 hours.

Original post

Why have load on the own server, if you could route it somewhere else?

Do some testing…

  • What does it "cost" to find the user and the cached ID from the database and generating the gravatar-url from it?
  • What does it "cost" to find the users email-address, generating the md5-hash and then the gravatar-url?
  • What does it "cost" to find the users cached file name, checking if it is updated at gravatar.com, fetching it if needed from there, update the cache, generate the url for the own server, deliver it in a new request…

Just remember, a gravatar should only be about a handfull of kilobytes or even less. Everyone tries to generate css-sprites for small images and bundle all the small images into one, just to save on requests and to reduce overhead data transfers, but when it comes to gravatars, suddenly everyone want to cache them… Even gravatar.com advices not to cache the gravatars somewhere else!

At least I remember it like that, but can't find it right now, but that seems to make sense! Consider everyone using gravatars and caching them on their own server and deliver them from there. The browser can't distinguish between all the same but different gravatars for its own user, so it fetches every single gravatar and downloads it into its own local caching storage. But if every site uses the gravatar.com URL for the image, the browser only checks every now and then for an update and flushes its cache on its own behalf.

Don't try to cache external services unless you have to!

So, yes, I would suppose to drop any caching of gravatars!

@atermenji
Copy link
Contributor

I don't know about GitHub server, we are caching images in Android app's file system.
Having the file system avatars cache allows to load avatars a lot faster than from the network.
The only drawback here is that cache is not updated when avatar is changed on gravatar.com, so the user have to manually remove all application's cache data.

@NobbZ
Copy link

NobbZ commented Jul 26, 2013

I'm not an expert or any way other related in android programming, but as I supposed in my last post, just cache it and leave it for a couple of hours and when the cache has expired, check if the etag has changed, if yes download and cache the new version of the avatar, if not just use the old cached image.

The etag is just there for this purpose.

What I can't say right from the spot is if gravatar uses the etag. But they were stupid if not.

@atermenji
Copy link
Contributor

this makes sense, I'll try this approach

@JakeWharton
Copy link

What cache headers do the gravatar servers return? The right way to cache would be to use a mechanism that simply honors traditional HTTP caching semantics. This problem has been solved for years by the underlying transport mechanism so using an HTTP client that properly honors it means that you have to do zero in-app logic.

@NobbZ
Copy link

NobbZ commented Jul 26, 2013

The etag is one of this solutions, even if it is the youngest, if it is
supported by gravatar, it fits better than the classic expire headers, bit
I think gravatar is using a combination of both.

Only trade off I realized so far is, that most http libraries I used so far
didn't respect the different caching headers, so I fear you have to self
implement it.

But as I said already I don't have experience in android development, so I
can't tell about the libs there.
Am 26.07.2013 18:26 schrieb "Jake Wharton" [email protected]:

What cache headers do the gravatar servers return? The right way to cache
would be to use a mechanism that simply honors traditional HTTP caching
semantics. This problem has been solved for years by the underlying
transport mechanism so using an HTTP client that properly honors it means
that you have to do zero in-app logic.


Reply to this email directly or view it on GitHubhttps://github.com//pull/349#issuecomment-21631743
.

@JakeWharton
Copy link

OkHttp supports it.

@atermenji
Copy link
Contributor

AvatarLoader is using HttpRequest library to download avatars.
HttpRequest allows to easily check eTags and it supports OkHttp also.
Hope to address this issue soon

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

Successfully merging this pull request may close these issues.

5 participants