Skip to content

Cache for Github Organization membership#3

Merged
kohsuke merged 5 commits intojenkinsci:masterfrom
spajus:github_cache
Jul 11, 2013
Merged

Cache for Github Organization membership#3
kohsuke merged 5 commits intojenkinsci:masterfrom
spajus:github_cache

Conversation

@spajus
Copy link
Contributor

@spajus spajus commented Feb 23, 2013

When Github Organization membership is used to check if users are allowed to perform actions, all non-admin users suffer from terrible UI performance, since every request to Jenkins does actual requests to Github to retrieve user memberships.

This patch adds a simple 24 hour long cache for logged in user organizations, offering a significant performance improvement for non-admin users.

This patch was built and tested with actual organization account, it works.

@buildhive
Copy link

Jenkins » github-oauth-plugin #13 SUCCESS
This pull request looks good
(what's this?)

@bbbco
Copy link

bbbco commented Apr 26, 2013

Please merge this item, as our users have complained about this!

@bjeanes
Copy link
Contributor

bjeanes commented Apr 29, 2013

👍

@ghost
Copy link

ghost commented Apr 29, 2013

@bbbco, see #4 (comment)

Also, 👍

@buildhive
Copy link

Jenkins » github-oauth-plugin #27 SUCCESS
This pull request looks good
(what's this?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would consider a different caching strategy to avoid possible OOM, something along the lines of..

private final Map<String, SoftReference<?>> userOrganizationCache = new LinkedHashMap<String, SoftReference<?>>() {
        @Override
        protected boolean removeEldestEntry(Map.Entry<String, SoftReference<?>> entry) {
            return size() > CACHE_CAPACITY;
          }
    };

then again, what about dirty cache? invalidation?

@skottler

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed patch is as simple as it is due to these reasons:

  • How many unique visitors would have to log in to run this map out of memory? Let's say one user entry costs 128 bytes. It's probably less, because username + one or two organizations (most likely zero for most users) should not take up that much. 1 Mb of RAM gives you ~8000 users. If you have more people working at your organization, you must be able to spare a few more megabytes of RAM. :)
  • How often do users leave / join organization? Cache will reset once a day and invalidate itself. And if you really want to cut someone off very quickly, restarting Jenkins is pretty easy.

Depends on what the use cases are, this solution will most likely work perfectly and no overengineering with full blown cache is required.

Anyway, feel free to take over and improve it if you want, I just shared what I did for our organization and what works there. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some thoughts:

How many unique visitors would have to log in to run this map out of memory? Let's say one user entry costs 128 bytes. It's probably less, because username + one or two organizations (most likely zero for most users) should not take up that much. 1 Mb of RAM gives you ~8000 users. If you have more people working at your organization, you must be able to spare a few more megabytes of RAM. :)

In the case of our GitHub Enterprise installation, organizations map loosely to teams and projects, not companies. Most users are members of 10-20 organizations easily...

How often do users leave / join organization? Catche will reset once a day and invalidate itself. And if you really want to cut someone off very quickly, restarting Jenkins is pretty easy.

Restarting Jenkins might be easy but usually is not viable during the day. At least with the number of projects, agents, plugins, and history of builds that we have, Jenkins is a 15+ min process...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, that makes sense. But even with 20 organizations thousands of users would hardly cause OOM. Still, I agree that cache eviction and possibly a "Clear Github organization cache" button in security settings would be nice to have.

I could eventually add them, but it will take me a while.

@buildhive
Copy link

Jenkins » github-oauth-plugin #32 FAILURE
Looks like there's a problem with this pull request
(what's this?)

@buildhive
Copy link

Jenkins » github-oauth-plugin #33 SUCCESS
This pull request looks good
(what's this?)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment looks outdated?

@praseodym praseodym mentioned this pull request Jun 10, 2013
@praseodym
Copy link

👍, please ship a fix for this issue. Our Jenkins install became unusable after activating this plugin because we hit the API rate limit when a couple of users were active. The plugin apparently does one GitHub API call per page request, which is a sure way to hit the limit.

@kohsuke kohsuke merged commit a8f578d into jenkinsci:master Jul 11, 2013
@spajus spajus deleted the github_cache branch July 11, 2013 15:56
@Memphiz
Copy link

Memphiz commented Dec 2, 2013

Are we sure that this didn't break anything? Maybe in conjunction with a change in jenkins itself? Since some days all users in our organisation (XBMC) are able to login but don't get any build options. They basically only have their profile and thats it. They can't kick builds and nothing.

Only admins seem to be able to see more options. Logged in Organisation members are treated like anonymous users.

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.

8 participants