Convert copy_source to in_copied_subtree, to account for the fact tha…

…t all shards in the `from` template should be preserved until post copy
twitter-archive · Feb 14, 2012 · 15e2c39 · 15e2c39
1 parent 7793eab
commit 15e2c39
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 18 deletions.
diff --git a/lib/gizzard/transformation.rb b/lib/gizzard/transformation.rb
@@ -181,15 +181,15 @@ def copies_required?
     end
 
     def involved_in_copy?(template)
-      copy_source?(template) || copy_destination?(template)
+      in_copied_subtree?(template) || copy_destination?(template)
     end
 
     def copy_destination?(template)
       copies_required? && template.concrete? && !from.shared_host?(template)
     end
 
-    def copy_source?(template)
+    def in_copied_subtree?(template)
-      copies_required? && !!from.copy_sources.find {|s| s.shard_eql? template }
+      copies_required? && !!from.descendants.find {|s| s.shard_eql? template }
     end
 
     def copy_source

diff --git a/test/transformation_spec.rb b/test/transformation_spec.rb
@@ -167,32 +167,32 @@ def empty_ops
       rebalancer.transformations.size.should == 1
       transformation = rebalancer.transformations.clone.shift[0]
       transformation.operations.should == empty_ops.merge({
-        :prepare => [ create_shard('WriteOnlyShard'),
+        :prepare => [ create_shard('SqlShard(host4)'),
                       create_shard('BlockedShard'),
-                      create_shard('SqlShard(host4)'),
                       create_shard('BlockedShard'),
                       create_shard('SqlShard(host3)'),
-                      add_link('WriteOnlyShard', 'BlockedShard'),
+                      create_shard('WriteOnlyShard'),
-                      add_link('BlockedShard', 'SqlShard(host3)'),
                       add_link('ReplicatingShard', 'WriteOnlyShard'),
+                      add_link('WriteOnlyShard', 'BlockedShard'),
                       add_link('BlockedShard', 'SqlShard(host4)'),
+                      add_link('BlockedShard', 'SqlShard(host3)'),
                       add_link('ReplicatingShard', 'BlockedShard') ],
-        :copy =>    [ copy_shard('SqlShard(host1)', 'SqlShard(host3)'),
+        :copy =>    [ copy_shard('SqlShard(host1)', 'SqlShard(host4)'),
-                      copy_shard('SqlShard(host1)', 'SqlShard(host4)') ],
+                      copy_shard('SqlShard(host1)', 'SqlShard(host3)') ],
-        :cleanup => [ add_link('ReplicatingShard', 'SqlShard(host3)'),
+        :cleanup => [ add_link('WriteOnlyShard', 'SqlShard(host4)'),
-                      add_link('WriteOnlyShard', 'SqlShard(host4)'),
+                      add_link('ReplicatingShard', 'SqlShard(host3)'),
-                      remove_link('ReplicatingShard', 'BlockedShard'),
+                      remove_link('ReplicatingShard', 'SqlShard(host1)'),
+                      remove_link('BlockedShard', 'SqlShard(host3)'),
+                      remove_link('ReplicatingShard', 'WriteOnlyShard'),
                       remove_link('WriteOnlyShard', 'BlockedShard'),
                       remove_link('BlockedShard', 'SqlShard(host4)'),
-                      remove_link('ReplicatingShard', 'SqlShard(host1)'),
                       remove_link('WriteOnlyShard', 'SqlShard(host2)'),
-                      remove_link('ReplicatingShard', 'WriteOnlyShard'),
+                      remove_link('ReplicatingShard', 'BlockedShard'),
-                      delete_shard('BlockedShard'),
-                      delete_shard('BlockedShard'),
                       delete_shard('SqlShard(host1)'),
-                      # FIXME: DATASERV-40: these deletes are currently occurring in :prepare
                       delete_shard('WriteOnlyShard'),
-                      delete_shard('SqlShard(host2)') ]
+                      delete_shard('BlockedShard'),
+                      delete_shard('SqlShard(host2)'),
+                      delete_shard('BlockedShard') ]
       })
     end
 
@@ -281,4 +281,19 @@ def collapse(jobs); @trans.collapse_jobs(jobs) end
       @trans.copy_destination?(@to_template).should == false
     end
   end
+
+  describe "in_copied_subtree?" do
+    it "returns true for copy sources" do
+      @trans.in_copied_subtree?(@host_2_template).should == true
+    end
+
+    it "returns true for members of a subtree that contains copy sources, but who are not copy sources" do
+      @trans.in_copied_subtree?(@host_1_template).should == true
+      @trans.in_copied_subtree?(@blocked_template).should == true
+    end
+
+    it "returns false for templates added in the destination" do
+      @trans.in_copied_subtree?(@host_3_template).should == false
+    end
+  end
 end