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
View
@@ -1,5 +1,5 @@
class Pusher
attr_reader :user, :spec, :message, :code, :rubygem, :body, :version, :version_id
attr_reader :user, :spec, :message, :code, :rubygem, :body, :version, :version_id, :size
def initialize(user, body, host_with_port=nil)
@user = user
@@ -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.
end
def authorize
@@ -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.

true
else
false
end
end
def pull_spec
Gem::Package.open body, "r", nil do |pkg|
@spec = pkg.metadata
@@ -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.

if @version.new_record?
true
View
@@ -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.

version = find_or_initialize_version_from_spec(spec)
version.size = size
version
end
def find_or_initialize_version_from_spec(spec)
version = self.versions.find_or_initialize_by_number_and_platform(spec.version.to_s, spec.original_platform.to_s)
version.rubygem = self
View
@@ -132,6 +132,14 @@ def yanked?
!indexed
end
def sized?
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.

size.bytes / 1.kilobyte
end
def info
[ description, summary, "This rubygem does not have a description or summary." ].detect(&:present?)
end
@@ -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!

<% end %>
<% if version.yanked? -%>
<span class="yanked"><%= t '.yanked' %></span>
<% end -%>
@@ -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.

end
def self.down
remove_column :versions, :size
end
end
View
@@ -12,7 +12,6 @@
# It's strongly recommended to check this file into your version control system.
ActiveRecord::Schema.define(:version => 20120916165331) do
create_table "announcements", :force => true do |t|
t.text "body"
t.datetime "created_at", :null => false
@@ -143,6 +142,7 @@
t.integer "position"
t.boolean "latest"
t.string "full_name"
t.integer "size"
end
add_index "versions", ["built_at"], :name => "index_versions_on_built_at"
View
@@ -11,6 +11,7 @@ Feature: Push Gems
And I visit the gem page for "RGem"
Then I should see "RGem"
And I should see "1.2.3"
And I should see "(3 KB)"
Scenario: User pushes existing version of existing gem
Given I am signed up as "email@person.com"
View
@@ -21,7 +21,7 @@ namespace :gemcutter do
puts "Processing #{path}"
cutter = Pusher.new(nil, File.open(path))
cutter.pull_spec and cutter.find and cutter.save
cutter.pull_spec and cutter.size_gem and cutter.find and cutter.save
end
end
end
@@ -744,7 +744,7 @@ table {
text-decoration: underline;
}
.main .info .meta .versions .platform {
.main .info .meta .versions .platform .size {
color: #B0A58C;
font-size: 0.85em;
margin-left: 4px;
@@ -0,0 +1,28 @@
#!/usr/bin/env ruby
ENV['RAILS_ENV'] ||= 'production'
require_relative '../config/environment'
def fog
$fog || Fog::Storage.new(:provider => 'Local',
:local_root => Pusher.server_path('gems'))
end
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.

.files.get("#{version.full_name}.gem")
if file
begin
size = file.content_length.bytes
version.update_attribute(:size, size)
print "succeeded (#{size})\n"
rescue => e
print "failed - #{e.message}"
end
else
print "failed - file not found"
end
end
end
View
@@ -81,6 +81,7 @@
number
platform "ruby"
rubygem
size 1024
end
factory :version_history do
View
@@ -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 :(

mock(@cutter).find { true }
stub(@cutter).authorize { true }
mock(@cutter).save
@@ -47,6 +48,7 @@ class PusherTest < ActiveSupport::TestCase
should "not attempt to find rubygem if spec can't be pulled" do
mock(@cutter).pull_spec { false }
mock(@cutter).size_gem.never
mock(@cutter).find.never
mock(@cutter).authorize.never
mock(@cutter).save.never
@@ -55,6 +57,7 @@ class PusherTest < ActiveSupport::TestCase
should "not attempt to authorize if not found" do
mock(@cutter).pull_spec { true }
mock(@cutter).size_gem { true }
mock(@cutter).find { nil }
mock(@cutter).authorize.never
mock(@cutter).save.never
@@ -64,6 +67,7 @@ class PusherTest < ActiveSupport::TestCase
should "not attempt to save if not authorized" do
mock(@cutter).pull_spec { true }
mock(@cutter).size_gem { true }
mock(@cutter).find { true }
mock(@cutter).authorize { false }
mock(@cutter).save.never
@@ -548,7 +548,7 @@ class RubygemTest < ActiveSupport::TestCase
Rubygem.create(:name => "rake")
@rubygem = Rubygem.new(:name => @specification.name)
@version = @rubygem.find_or_initialize_version_from_spec(@specification)
@version = @rubygem.find_or_initialize_version_from_spec_and_size(@specification, 5)
end
should "save the gem" do
@@ -561,6 +561,7 @@ class RubygemTest < ActiveSupport::TestCase
assert_equal 1, @rubygem.versions.count
assert_equal 1, @rubygem.versions_count
assert_equal 2, @version.dependencies.count
assert_equal 5, @version.size
assert Rubygem.exists?(:name => 'thoughtbot-shoulda')
assert Rubygem.exists?(:name => 'rake')
end
@@ -150,6 +150,10 @@ class VersionTest < ActiveSupport::TestCase
assert ! @version.platformed?
end
should "return the size in kilobytes" do
assert (@version.size.bytes / 1.kilobyte) == @version.size_in_kilobytes
end
should "save full name" do
assert_equal "#{@version.rubygem.name}-#{@version.number}", @version.full_name
assert_equal @version.number, @version.slug
ProTip! Use n and p to navigate between commits in a pull request.