-
Notifications
You must be signed in to change notification settings - Fork 405
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
Faster way to add roles #468
Conversation
This method is much faster than `role_ids |= [role.id]`. .include? does a fast exists check without having to load all the ids.
Update role_adapter.rb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hi @v-kumar, the test suite is not kicking off on Travis-ci that raises a "Abuse detected" error. I got in touch with Travis-ci support that told me the organization you used (Ogwee) to create the fork never logged in Travis-ci and may have trigger this false positive in their abuse detection process.
Can you try to login with your github account on Travis to unblock the PR? Or if you can't/don't want to, you can re-create a fork using your personal account, not the Ogwee organization, that maybe would do the trick. Thanks |
Hi @EppO Sorry, it took a while for me to see your earlier message. I just granted the access to travis-ci. You should be all set now. Let me know once this gets merged, |
3 similar comments
I'm investigating similar performance issues with one of our applications, it would be great to get this merged in. Is there anything I can do to help this land? |
The CI failed because it ran out of time. Can we please get this merged? |
Merge done. Thanks for you PR. really appreciated |
Looks like this PR has been merged into the master branch, but it's not in the latest gem release (5.2.0), and it's not even tagged. The only way for us to get this fix (which is very important for us - we're seeing 60+ seconds for role creations in some cases) is to point our Gemfile at this specific commit, which we'd prefer not to do if possible. We'd like to at least be able to point at a specific release (although ideally you'd just create a tag and then push that tag to rubygems to make the fix available to everyone). Any way a new gem release could be done, or at least a new tagged release in GitHub we could point to? |
This method is much faster than role_ids |= [role.id].
.include? does a fast exists check without having to load all the ids. The slowness is not in the insert, but in loading all the role_ids, which is not needed here.