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

Add size to versions #484

Merged
merged 7 commits into from Jul 12, 2013

Conversation

Projects
None yet
7 participants
@gnarg
Contributor

gnarg commented Nov 4, 2012

Up to date version of cmeiklejohn's pull request #310

Show outdated Hide outdated app/models/pusher.rb
@@ -28,6 +28,15 @@ def save
end
end
def size_gem
if body
@size = body.size

This comment has been minimized.

@Skipants

Skipants Nov 4, 2012

You could probably memoize this

@size ||= body.size
@Skipants

Skipants Nov 4, 2012

You could probably memoize this

@size ||= body.size

This comment has been minimized.

@cmeiklejohn

cmeiklejohn Nov 7, 2012

Contributor

Not sure it's really necessary if we only use it once.

@cmeiklejohn

cmeiklejohn Nov 7, 2012

Contributor

Not sure it's really necessary if we only use it once.

Show outdated Hide outdated app/models/version.rb
size.present?
end
def size_in_kilobytes

This comment has been minimized.

@adkron

adkron Nov 6, 2012

Contributor

This method is only used in the tests. I think it should be removed.

@adkron

adkron Nov 6, 2012

Contributor

This method is only used in the tests. I think it should be removed.

This comment has been minimized.

@cmeiklejohn

cmeiklejohn Nov 6, 2012

Contributor

+1

@cmeiklejohn

cmeiklejohn Nov 6, 2012

Contributor

+1

This comment has been minimized.

@qrush

qrush Nov 7, 2012

Member

Let's kill it.

@qrush

qrush Nov 7, 2012

Member

Let's kill it.

@cmeiklejohn cmeiklejohn referenced this pull request Nov 6, 2012

Closed

Add size to versions. #310

Show outdated Hide outdated app/views/versions/_version.html.erb
@@ -4,6 +4,9 @@
<% if version.platformed? %>
<span class="platform"><%= version.platform %></span>
<% end %>
<% if version.sized? %>
<span class="size">(<%= number_to_human_size(version.size) %>)</span>

This comment has been minimized.

@adkron

adkron Nov 6, 2012

Contributor

Can we get rid of the if and make version.size return "NA" if there is no size? It would follow tell don't ask, and make the view more consistant on how it looks?

@adkron

adkron Nov 6, 2012

Contributor

Can we get rid of the if and make version.size return "NA" if there is no size? It would follow tell don't ask, and make the view more consistant on how it looks?

This comment has been minimized.

@qrush

qrush Nov 7, 2012

Member

I'm fine with this approach. N/A sounds fine to me!

@qrush

qrush Nov 7, 2012

Member

I'm fine with this approach. N/A sounds fine to me!

@adkron

This comment has been minimized.

Show comment
Hide comment
@adkron

adkron Nov 7, 2012

Contributor

@gnarg Are you still working on this or do you need someone else to finish it off? I don't want to sweep in if someone is actively working on this.

Contributor

adkron commented Nov 7, 2012

@gnarg Are you still working on this or do you need someone else to finish it off? I don't want to sweep in if someone is actively working on this.

@gnarg

This comment has been minimized.

Show comment
Hide comment
@gnarg

gnarg Nov 7, 2012

Contributor

Hi, sorry, yeah, I'll incorporate the suggestions here and resubmit, hopefully today.

I was trying to get caught up after RubyConf, but I think I have some time now. The Hack-a-thon / BoF thing at RubyConf was a lot of fun and I'd like to get this landed if possible.

Thanks.

Contributor

gnarg commented Nov 7, 2012

Hi, sorry, yeah, I'll incorporate the suggestions here and resubmit, hopefully today.

I was trying to get caught up after RubyConf, but I think I have some time now. The Hack-a-thon / BoF thing at RubyConf was a lot of fun and I'd like to get this landed if possible.

Thanks.

@cmeiklejohn

This comment has been minimized.

Show comment
Hide comment
@cmeiklejohn

cmeiklejohn Nov 7, 2012

Contributor

Cool, just let us know when it's good to go, we'll get it merged and coordinate the deployment with the rake task to backfill the version numbers. Thanks!

Contributor

cmeiklejohn commented Nov 7, 2012

Cool, just let us know when it's good to go, we'll get it merged and coordinate the deployment with the rake task to backfill the version numbers. Thanks!

@gnarg

This comment has been minimized.

Show comment
Hide comment
@gnarg

gnarg Nov 8, 2012

Contributor

Ok, I think I incorporated all the given suggestions. Should be ok to merge unless there are more ideas for improvements.

Contributor

gnarg commented Nov 8, 2012

Ok, I think I incorporated all the given suggestions. Should be ok to merge unless there are more ideas for improvements.

@adkron

This comment has been minimized.

Show comment
Hide comment
@adkron

adkron Nov 8, 2012

Contributor

:shipit: or 🚢 it
Ship It!

Contributor

adkron commented Nov 8, 2012

:shipit: or 🚢 it
Ship It!

Version.find_in_batches do |versions|
versions.each do |version|
print "sizing #{version.full_name}: "
file = fog.directories.get($rubygems_config[:s3_bucket] + '/gems') \

This comment has been minimized.

@qrush

qrush Nov 8, 2012

Member

This... is going to take a really, really long time.

1.9.3p0 :001 > Version.indexed.count
   (172.1ms)  SELECT COUNT(*) FROM "versions" WHERE "versions"."indexed" = 't'
 => 253733

Maybe there's a better way we can do this with S3? Some kind of export of data? /cc @geemus

@qrush

qrush Nov 8, 2012

Member

This... is going to take a really, really long time.

1.9.3p0 :001 > Version.indexed.count
   (172.1ms)  SELECT COUNT(*) FROM "versions" WHERE "versions"."indexed" = 't'
 => 253733

Maybe there's a better way we can do this with S3? Some kind of export of data? /cc @geemus

This comment has been minimized.

@cmeiklejohn

cmeiklejohn Nov 8, 2012

Contributor

Why can't we roll some ec2 instances to compute the sizes of all of the gems on s3 and create a file of sizes and then use that to import on the main gemcutter instances? This way, i/o to AWS is free for all of the EC2 operations and we can leave it running somewhere without worrying about site performance?

@cmeiklejohn

cmeiklejohn Nov 8, 2012

Contributor

Why can't we roll some ec2 instances to compute the sizes of all of the gems on s3 and create a file of sizes and then use that to import on the main gemcutter instances? This way, i/o to AWS is free for all of the EC2 operations and we can leave it running somewhere without worrying about site performance?

This comment has been minimized.

@evanphx

evanphx Nov 8, 2012

Member

We can absolutely spin up EC2 instances to update the sizes. That can be done async after this change is merged in.

@evanphx

evanphx Nov 8, 2012

Member

We can absolutely spin up EC2 instances to update the sizes. That can be done async after this change is merged in.

Show outdated Hide outdated test/unit/pusher_test.rb
@@ -38,6 +38,7 @@ class PusherTest < ActiveSupport::TestCase
context "processing incoming gems" do
should "work normally when things go well" do
mock(@cutter).pull_spec { true }
mock(@cutter).size_gem { true }

This comment has been minimized.

@qrush

qrush Nov 8, 2012

Member

I really hate this test now :(

@qrush

qrush Nov 8, 2012

Member

I really hate this test now :(

@@ -0,0 +1,9 @@
class AddSizeToVersion < ActiveRecord::Migration
def self.up
add_column :versions, :size, :integer

This comment has been minimized.

@qrush

qrush Nov 8, 2012

Member

Anyone know how Postgres is going to handle this? Do we need to plan for downtime? /cc @evanphx @tcopeland

@qrush

qrush Nov 8, 2012

Member

Anyone know how Postgres is going to handle this? Do we need to plan for downtime? /cc @evanphx @tcopeland

This comment has been minimized.

@cmeiklejohn

cmeiklejohn Nov 8, 2012

Contributor

As far as I know, this should not be a problem. The entire table is only rewritten if you add a not-null constraint or change the type of an existing column.

@cmeiklejohn

cmeiklejohn Nov 8, 2012

Contributor

As far as I know, this should not be a problem. The entire table is only rewritten if you add a not-null constraint or change the type of an existing column.

Show outdated Hide outdated app/models/rubygem.rb
@@ -230,6 +230,12 @@ def yank!(version)
end
end
def find_or_initialize_version_from_spec_and_size(spec, size)

This comment has been minimized.

@qrush

qrush Nov 8, 2012

Member

This is only used from Pusher. I don't see why we just can't have just this method and merge the code from find_or_initialize_version_from_spec in here.

@qrush

qrush Nov 8, 2012

Member

This is only used from Pusher. I don't see why we just can't have just this method and merge the code from find_or_initialize_version_from_spec in here.

Show outdated Hide outdated app/models/pusher.rb
@@ -9,7 +9,7 @@ def initialize(user, body, host_with_port=nil)
end
def process
pull_spec && find && authorize && save
pull_spec && size_gem && find && authorize && save

This comment has been minimized.

@qrush

qrush Nov 8, 2012

Member

Huge HUGE 👎 to processing gem size as part of this method. It should be in update instead.

A few reasons for this:

  1. We don't need to calculate the size yet until we know the gem has been found and authorized to push.
  2. This really ruins the flow of this method...it's not "core" to the processing of a gem.
@qrush

qrush Nov 8, 2012

Member

Huge HUGE 👎 to processing gem size as part of this method. It should be in update instead.

A few reasons for this:

  1. We don't need to calculate the size yet until we know the gem has been found and authorized to push.
  2. This really ruins the flow of this method...it's not "core" to the processing of a gem.
@qrush

This comment has been minimized.

Show comment
Hide comment
@qrush

qrush Nov 8, 2012

Member

Reviewed, this is not ready to merge yet.

Member

qrush commented Nov 8, 2012

Reviewed, this is not ready to merge yet.

Show outdated Hide outdated app/models/pusher.rb
@@ -46,7 +55,7 @@ def pull_spec
def find
@rubygem = Rubygem.find_or_initialize_by_name(spec.name)
@version = @rubygem.find_or_initialize_version_from_spec(spec)
@version = @rubygem.find_or_initialize_version_from_spec_and_size(spec, size)

This comment has been minimized.

@evanphx

evanphx Nov 8, 2012

Member

Why would we use the size to look up the version? Why would we create the same version with different sizes?

@evanphx

evanphx Nov 8, 2012

Member

Why would we use the size to look up the version? Why would we create the same version with different sizes?

This comment has been minimized.

@evanphx

evanphx Nov 8, 2012

Member

Oh, I see. The method is named badly, it doesn't actually use the size to search. As @qrush says below, nuke this method.

@evanphx

evanphx Nov 8, 2012

Member

Oh, I see. The method is named badly, it doesn't actually use the size to search. As @qrush says below, nuke this method.

@gnarg

This comment has been minimized.

Show comment
Hide comment
@gnarg

gnarg Nov 9, 2012

Contributor

I cleaned up some of the objectionable code. @cmeiklejohn I hope you don't mind too much that I'm basically rewriting your code here.

Contributor

gnarg commented Nov 9, 2012

I cleaned up some of the objectionable code. @cmeiklejohn I hope you don't mind too much that I'm basically rewriting your code here.

@gnarg

This comment has been minimized.

Show comment
Hide comment
@gnarg

gnarg Nov 15, 2012

Contributor

Any more feedback on this? It's worth noting that the travis failure was a transient github outtage:

https://travis-ci.org/rubygems/rubygems.org/jobs/3124922

Contributor

gnarg commented Nov 15, 2012

Any more feedback on this? It's worth noting that the travis failure was a transient github outtage:

https://travis-ci.org/rubygems/rubygems.org/jobs/3124922

@adkron

This comment has been minimized.

Show comment
Hide comment
@adkron

adkron Nov 28, 2012

Contributor

@qrush What should be done on this to make it ready?

Contributor

adkron commented Nov 28, 2012

@qrush What should be done on this to make it ready?

@zenspider

This comment has been minimized.

Show comment
Hide comment

Bueller?

@evanphx

This comment has been minimized.

Show comment
Hide comment
@evanphx

evanphx Jul 12, 2013

Member

Merging and deploying tomorrow (7/12/2013)

Member

evanphx commented Jul 12, 2013

Merging and deploying tomorrow (7/12/2013)

@evanphx evanphx merged commit ebbc2b6 into rubygems:master Jul 12, 2013

1 check failed

default The Travis build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment