Migrate Picture content from BinaryBlobs to Pictures table #153

bdunne · 2018-01-11T21:24:52Z

To be merged with: ManageIQ/manageiq#16810

https://bugzilla.redhat.com/show_bug.cgi?id=1532286

bdunne · 2018-01-11T21:59:38Z

@miq-bot add_labels core enhancement gaprindashvili/yes

miq-bot · 2018-01-11T22:00:23Z

@bdunne Cannot apply the following label because they are not recognized: core enhancement gaprindashvili/yes

Fryguy · 2018-01-16T19:12:45Z

db/migrate/20180111162442_move_pictures_blobs_to_pictures.rb

@@ -0,0 +1,51 @@
+class MovePicturesBlobsToPictures < ActiveRecord::Migration[5.0]
+  class BinaryBlob < ActiveRecord::Base
+    has_many :binary_blob_parts, -> { order(:id) }, :dependent => :delete_all


needs the :class designation to ensure it uses the stub.

Fryguy · 2018-01-16T19:13:06Z

db/migrate/20180111162442_move_pictures_blobs_to_pictures.rb

+class MovePicturesBlobsToPictures < ActiveRecord::Migration[5.0]
+  class BinaryBlob < ActiveRecord::Base
+    has_many :binary_blob_parts, -> { order(:id) }, :dependent => :delete_all
+    belongs_to :resource, :polymorphic => true


You don't need this.

Fryguy · 2018-01-16T19:16:51Z

db/migrate/20180111162442_move_pictures_blobs_to_pictures.rb

+    add_column :pictures, :md5, :string
+
+    say_with_time("Moving picture content from BinaryBlobs to the pictures table") do
+      BinaryBlob.where(:resource_type => "Picture").all.each do |blob|


Let's change this to a find_each just to ensure we batch it properly just in case there are lot of these.

Also add a .includes(:binary_blob_parts) to avoid an N+1?

Also also do you need in_my_region? Even though they are not replicated, someone might have enabled that.

Fryguy · 2018-01-16T19:18:24Z

db/migrate/20180111162442_move_pictures_blobs_to_pictures.rb

+      Picture.in_my_region.all.each do |picture|
+        size = picture.content.try(:length).to_i
+
+        BinaryBlob.create!(


We shouldn't create the binaryblob if the size is 0. Though that seems like a real edge case.

https://bugzilla.redhat.com/show_bug.cgi?id=1532286

miq-bot · 2018-01-16T20:40:40Z

Checked commit bdunne@84cd462 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

himdel · 2018-01-18T14:06:34Z

== 20180111162442 MovePicturesBlobsToPictures: migrating ======================          
-- add_column(:pictures, :content, :binary)                                              
   -> 0.0045s                                                                            
-- add_column(:pictures, :extension, :string)
   -> 0.0005s
-- add_column(:pictures, :md5, :string)
   -> 0.0010s
-- Moving picture content from BinaryBlobs to the pictures table
rails aborted!
StandardError: An error has occurred, this and all later migrations canceled:

Couldn't find MovePicturesBlobsToPictures::Picture without an ID
bin/rails:4:in `require'
bin/rails:4:in `<main>'
ActiveRecord::RecordNotFound: Couldn't find MovePicturesBlobsToPictures::Picture without an ID
bin/rails:4:in `require'
bin/rails:4:in `<main>'
Tasks: TOP => db:migrate                                                                 
(See full trace by running task with --trace)                                            
                                                                                         
== Command ["bin/rails db:migrate"] failed in /home/himdel/manageiq ==

himdel · 2018-01-18T14:13:34Z

happens during up, blob looks like this:

#<MovePicturesBlobsToPictures::BinaryBlob id: 10000000014757, resource_type: "Picture", resource_id: nil, md5: "7d4b8a5fbbdfdd85da991e00e22e1545", size: 14853, part_size: 1048576, name: nil, data_type: "jpeg">
#<MovePicturesBlobsToPictures::BinaryBlob id: 10000000014758, resource_type: "Picture", resource_id: nil, md5: "fe95acfc63996cc4610c83273308b692", size: 17648, part_size: 1048576, name: nil, data_type: "jpeg">

(just these 2 fail in my db)

Not sure if there can be other links to the blobs and these are still used or if this is an aritfact of something not getting deleted and we should just drop these..?

Fryguy · 2018-01-18T16:05:42Z

How weird that you have nil pictures...that should not be possible, unless the original table was dependent => nullify?

himdel · 2018-01-18T16:10:02Z

It could theoretically have to do with updating GenericObjectDefintion via the API to remove the picture.

Looks like it does something along the lines of god.update_attributes!(:picture => nil).
But I'm not really sure if it's a problem still present in the current implementation or if this only happened during review.

simaishi · 2018-01-18T16:34:32Z

I'm holding off backporting to Gaprindashvili. Let me know if/when it's ok to backport.

Fryguy · 2018-01-18T16:48:33Z

@jntullo ?

Fryguy · 2018-01-18T16:49:23Z

@bdunne Can you update the PR to "deal with" resource_id being nil? However, separately, we should not be putting the DB in this state, so let's also fix whatever bug is causing this to happen.

jntullo · 2018-01-18T17:06:50Z

@Fryguy will look into that - also look into other seemingly related spec failures in the API.

bdunne · 2018-01-18T17:33:21Z

@simaishi I think this is all set for backport with #158

Migrate Picture content from BinaryBlobs to Pictures table (cherry picked from commit 91b0fc0) https://bugzilla.redhat.com/show_bug.cgi?id=1536046

simaishi · 2018-01-18T17:45:42Z

Gaprindashvili backport details:

$ git log -1
commit 6faac0b77f8d0ca90015ad6be847fcd52f4987e0
Author: Jason Frey <[email protected]>
Date:   Wed Jan 17 16:21:22 2018 -0500

    Merge pull request #153 from bdunne/move_pictures
    
    Migrate Picture content from BinaryBlobs to Pictures table
    (cherry picked from commit 91b0fc0430a7ebe601960be693ffeddf2ce842b2)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1536046

miq-bot added the wip label Jan 11, 2018

bdunne mentioned this pull request Jan 11, 2018

Picture content is moving to the pictures table. ManageIQ/manageiq#16810

Merged

bdunne changed the title ~~[WIP] Migrate Picture content from BinaryBlobs to Pictures table~~ Migrate Picture content from BinaryBlobs to Pictures table Jan 11, 2018

miq-bot removed the wip label Jan 11, 2018

Fryguy reviewed Jan 16, 2018

View reviewed changes

Fryguy added data schema enhancement labels Jan 16, 2018

Migrate Picture content from BinaryBlobs to Pictures table

84cd462

https://bugzilla.redhat.com/show_bug.cgi?id=1532286

bdunne force-pushed the move_pictures branch from 2e1c431 to 84cd462 Compare January 16, 2018 20:32

chessbyte added blocker gaprindashvili/yes labels Jan 17, 2018

chessbyte assigned Fryguy Jan 17, 2018

Fryguy merged commit 91b0fc0 into ManageIQ:master Jan 17, 2018

Fryguy added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 17, 2018

bdunne deleted the move_pictures branch January 17, 2018 21:59

bdunne mentioned this pull request Jan 18, 2018

Delete the dangling blob when there is a blob without a picture #158

Merged

simaishi pushed a commit that referenced this pull request Jan 18, 2018

Merge pull request #153 from bdunne/move_pictures

6faac0b

Migrate Picture content from BinaryBlobs to Pictures table (cherry picked from commit 91b0fc0) https://bugzilla.redhat.com/show_bug.cgi?id=1536046

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jan 18, 2018

bdunne mentioned this pull request Jan 18, 2018

Handle cases where a Picture with that id doesn't exist #159

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Picture content from BinaryBlobs to Pictures table #153

Migrate Picture content from BinaryBlobs to Pictures table #153

bdunne commented Jan 11, 2018 •

edited

Loading

bdunne commented Jan 11, 2018

miq-bot commented Jan 11, 2018

Fryguy Jan 16, 2018

Fryguy Jan 16, 2018

Fryguy Jan 16, 2018

Fryguy Jan 16, 2018

Fryguy Jan 16, 2018

Fryguy Jan 16, 2018

miq-bot commented Jan 16, 2018

himdel commented Jan 18, 2018

himdel commented Jan 18, 2018 •

edited

Loading

Fryguy commented Jan 18, 2018

himdel commented Jan 18, 2018

simaishi commented Jan 18, 2018

Fryguy commented Jan 18, 2018

Fryguy commented Jan 18, 2018

jntullo commented Jan 18, 2018

bdunne commented Jan 18, 2018

simaishi commented Jan 18, 2018

Migrate Picture content from BinaryBlobs to Pictures table #153

Migrate Picture content from BinaryBlobs to Pictures table #153

Conversation

bdunne commented Jan 11, 2018 • edited Loading

bdunne commented Jan 11, 2018

miq-bot commented Jan 11, 2018

Fryguy Jan 16, 2018

Choose a reason for hiding this comment

Fryguy Jan 16, 2018

Choose a reason for hiding this comment

Fryguy Jan 16, 2018

Choose a reason for hiding this comment

Fryguy Jan 16, 2018

Choose a reason for hiding this comment

Fryguy Jan 16, 2018

Choose a reason for hiding this comment

Fryguy Jan 16, 2018

Choose a reason for hiding this comment

miq-bot commented Jan 16, 2018

himdel commented Jan 18, 2018

himdel commented Jan 18, 2018 • edited Loading

Fryguy commented Jan 18, 2018

himdel commented Jan 18, 2018

simaishi commented Jan 18, 2018

Fryguy commented Jan 18, 2018

Fryguy commented Jan 18, 2018

jntullo commented Jan 18, 2018

bdunne commented Jan 18, 2018

simaishi commented Jan 18, 2018

bdunne commented Jan 11, 2018 •

edited

Loading

himdel commented Jan 18, 2018 •

edited

Loading