Skip to content

Remove yanked versions from reverse_dependencies#1104

Merged
sferik merged 1 commit into
rubygems:masterfrom
olivierlacan:exclude-yanked-gems-from-reverse-deps
Nov 10, 2015
Merged

Remove yanked versions from reverse_dependencies#1104
sferik merged 1 commit into
rubygems:masterfrom
olivierlacan:exclude-yanked-gems-from-reverse-deps

Conversation

@olivierlacan
Copy link
Copy Markdown
Contributor

This is something I remember surprising me when reading @schneems' http://www.schneems.com/blogs/2015-09-30-reverse-rubygems/.

Why did he need to rescue while querying the API for gems depending on the gem he was searching for?

I got my answer when I tried to run reverse dependencies for rake and hundreds of exceptions popped up for gems that have been yanked altogether, I'm not sure why.

Skipping yanked versions will probably speed up this endpoint a bunch although it's hard for me to test at this point. I can just log for posterity that before this commit the following requests total response times were as follows:

Please do let me know if I'm completely out of my depths here, since I don't know much more about version yanking as what I could gather from going through the Version and Deletion code. I still don't understand what causes a gem to be removed from the index entirely but I'm guessing that's when all its versions are yanked? /cc @qrush

@olivierlacan
Copy link
Copy Markdown
Contributor Author

I just realized after posting this that this may need to go into the v2 API only since it's altering behavior people may have been relying on. Will fix accordingly tomorrow.

@olivierlacan
Copy link
Copy Markdown
Contributor Author

@sferik Merged master to account for your merging of my other recent PRs. Curious what you think as well.

@sferik
Copy link
Copy Markdown
Member

sferik commented Nov 9, 2015

@olivierlacan I’d prefer you rebase from master and force push, so there’s no merge commit.

@sferik
Copy link
Copy Markdown
Member

sferik commented Nov 9, 2015

@olivierlacan After you do that, I’m 👍 overall.

@sferik
Copy link
Copy Markdown
Member

sferik commented Nov 9, 2015

I’m not worried about breaking API v1. Nobody should be relying on this behavior.

@schneems
Copy link
Copy Markdown

schneems commented Nov 9, 2015

Awesome job 🚀

@olivierlacan olivierlacan force-pushed the exclude-yanked-gems-from-reverse-deps branch from dd5b894 to d0b5280 Compare November 9, 2015 18:06
@olivierlacan
Copy link
Copy Markdown
Contributor Author

@sferik Rebased. It does seem that this API is under-used. I hope I can eventually change that. :-)

@schneems Thanks again for thought-leadering on this. 😍

Comment thread test/unit/rubygem_test.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@olivierlacan It's not a big deal but I think "except yanked versions" doesn't need parenthesis.

This is something I remember surprising me when reading @schneems'
http://www.schneems.com/blogs/2015-09-30-reverse-rubygems/.

Why did he need to rescue while querying the API for gems depending
on the gem he was searching for?

I got my answer when I tried to run reverse dependencies for rake and
hundreds of exceptions popped up for gems that have been yanked
altogether, not sure why.

Skipping yanked versions will probably speed up this endpoint a bunch
although it's hard for me to test at this point.

I can just log for posterity that before this commit the following requests
total response times were as follows:

- https://rubygems.org/api/v1/gems/rake/reverse_dependencies.json => 4.21 s (X-Runtime: 1.649987)
- https://rubygems.org/api/v1/gems/rails/reverse_dependencies.json => 2.82 s (X-Runtime: 0.522366)
- https://rubygems.org/api/v1/gems/bundler/reverse_dependencies.json => 3.89 s (X-Runtime: 1.636216)
- https://rubygems.org/api/v1/gems/rack/reverse_dependencies.json => 1.97 s (X-Runtime: 0.188214)
@olivierlacan olivierlacan force-pushed the exclude-yanked-gems-from-reverse-deps branch from d0b5280 to 2e105ee Compare November 10, 2015 00:01
@olivierlacan
Copy link
Copy Markdown
Contributor Author

Thanks to the handy data dumps, I've now tested this locally and it works great. Not a single API error when fetching all listed reverse_dependencies for rack.

The gem count goes from 2577 to 2454 gems for rack, so there are 123 "dead" (all versions yanked?) gems no long being returned.

sferik added a commit that referenced this pull request Nov 10, 2015
…everse-deps

Remove yanked versions from reverse_dependencies
@sferik sferik merged commit 4d54ef3 into rubygems:master Nov 10, 2015
@sferik
Copy link
Copy Markdown
Member

sferik commented Nov 10, 2015

Nice!

@sferik
Copy link
Copy Markdown
Member

sferik commented Nov 10, 2015

Deployed. 🚀

@olivierlacan
Copy link
Copy Markdown
Contributor Author

Yay! Thanks Erik!

On Nov 10, 2015, at 11:42 AM, Erik Michaels-Ober notifications@github.com wrote:

Deployed.


Reply to this email directly or view it on GitHub.

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.

4 participants