On 21/03/10 02:52, Markus Roberts wrote:
> When I test applied this branch this morning pre-coffee I resolved the
> following conflict in favor of Brice's branch
>
> <<<<<<< Luke's:
> eval_children_and_apply_resource(resource)
>
> # Check to see if there are any events queued for this resource
> event_manager.process_events(resource)
> ||||||| Master:
> # Collect the targets of any subscriptions to those events. We pass
> # the parent resource in so it will override the source in the
> events,
> # since eval_generated children can't have direct relationships.
> relationship_graph.matching_edges(events, resource).each do
> |orig_edge|
> # We have to dup the label here, else we modify the original
> edge label,
> # which affects whether a given event will match on the next
> run, which is,
> # of course, bad.
> edge = orig_edge.class.new(orig_edge.source, orig_edge.target,
> orig_edge.label)
> edge.event = events.collect { |e| e.name }
> set_trigger(edge)
> end
>
> # And return the events for collection
> events
> ======= Brices:
> # Collect the targets of any subscriptions to those events. We pass
> # the parent resource in so it will override the source in the
> events,
> # since eval_generated children can't have direct relationships.
> Puppet::Util.benchmark(:debug, "Time for triggering #{events.size}
> events to edges") do
> b = relationship_graph.matching_edges(events, resource)
> b.each do |orig_edge|
> # We have to dup the label here, else we modify the original
> edge label,
> # which affects whether a given event will match on the next
> run, which is,
> # of course, bad.
> edge = orig_edge.class.new(orig_edge.source,
> orig_edge.target, orig_edge.label)
> edge.event = events.collect { |e| e.name }
> set_trigger(edge)
> end
> end
> # And return the events for collection
> events
>>>>>>>>
>
> This caused enough confusion down the line that I didn't include the patch
> in the testing branch, pending resolution. Coming back to it this afternoon
> I realized that Brice's change just adds the benchmarks and Luke's actually
> has the stuff we want.
Yes, I added the benchmark as a way to monitor if we did progress or
not. It's absolutely not necessary and we (at least I) should remove it.
Especially if that makes merging easier.
> With that resolution things apply cleanly but we get four new test failures:
>
> 1)
> NoMethodError in 'Puppet::SimpleGraph when matching edges should match edges
> whose source matches the source of the event'
> undefined method `inject' for #<Puppet::Transaction::Event:0x274f9f8>
> /Users/markus/projects/puppet/lib/puppet/simple_graph.rb:159:in
> `matching_edges'
> spec/unit/simple_graph.rb:361:
>
> 2)
> NoMethodError in 'Puppet::SimpleGraph when matching edges should match
> always match nothing when the event is :NONE'
> undefined method `inject' for #<Puppet::Transaction::Event:0x274d770>
> /Users/markus/projects/puppet/lib/puppet/simple_graph.rb:159:in
> `matching_edges'
> spec/unit/simple_graph.rb:365:
>
> 3)
> NoMethodError in 'Puppet::SimpleGraph when matching edges should match
> multiple edges'
> undefined method `inject' for #<Puppet::Transaction::Event:0x274ad7c>
> /Users/markus/projects/puppet/lib/puppet/simple_graph.rb:159:in
> `matching_edges'
> spec/unit/simple_graph.rb:370:
>
> 4)
> NoMethodError in 'Puppet::SimpleGraph when matching edges from generated
> resources should not match with edges pointing back to events sources'
> undefined method `source' for #<Puppet::Transaction::Event:0x2748248>
> /Users/markus/projects/puppet/lib/puppet/simple_graph.rb:156:in
> `matching_edges'
> /Library/Ruby/Gems/1.8/gems/mocha-0.9.7/lib/mocha/class_method.rb:40:in
> `inject'
> /Users/markus/projects/puppet/lib/puppet/simple_graph.rb:156:in `each'
> /Users/markus/projects/puppet/lib/puppet/simple_graph.rb:156:in `inject'
> /Users/markus/projects/puppet/lib/puppet/simple_graph.rb:156:in
> `matching_edges'
> spec/unit/simple_graph.rb:385:
>
> Finished in 0.127558 seconds
>
> 64 examples, 4 failures
>
> The first three stem from matching_edges getting a single event when it
> expects a collection, and the last I'm not clear on.
Yes, the first 3 are failing because they're waiting for an array but
Luke's patch sends events one by one (apparently, I didn't re-read the
code).
The last one is more problematic, since if Luke removed the source
accessor from the event, we won't be able to prune our relationship
graph (which is where we got all the gain) in the case of the recursive
file resource.
> From Luke's comments
> on the cage-match thread:
>
>
>> It's unfortunately not that simple - if I'm reading this correctly, I
>> changed the method profile in 'matching_edges' to accept just one event
>> instead of a list of events.
>>
>> In looking briefly at my code, I've split what a moderately ugly but
>> all-in-one-place method into a bunch of different methods, and I think that
>> splitting quite likely invalidated Brice's patch.
>>
>> In looking briefly, I think the fix is to change EventManager#queue_event
>> to accept an array of events, and then change SimpleGraph#matching_edges
>> back to accepting an array, also. This should allow Brice's patch to apply
>> relatively cleanly, I think.
>>
>
> I gather that when I get that patch from him it should all go smoothly, with
> the possible exception of the event.source test. Does that sound correct?
I have the same analysis. I'm going to read Luke's event patch again
(was it published on puppet-dev?) and see what happened to this source
accessor.
--
Brice Figureau
My Blog: http://www.masterzen.fr/
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to puppet-...@googlegroups.com.
To unsubscribe from this group, send email to
puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
