On Mon, Jun 6, 2011 at 12:25, R.I.Pienaar <r...@devco.net> wrote: > ----- Original Message ----- >> > >> > diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb >> > index cfeb73a..00c3b18 100644 >> > --- a/lib/puppet/configurer.rb >> > +++ b/lib/puppet/configurer.rb >> > @@ -156,9 +156,10 @@ class Puppet::Configurer >> > return >> > end >> > ensure >> > - # Make sure we forget the retained module_directories of any >> > autoload >> > - # we might have used. >> > + # Make sure we forget the retained module_directories and gem >> > search >> > + # path of any autoload we might have used. >> > Thread.current[:env_module_directories] = nil >> > + Thread.current[:gemsearch_directories] = nil >> >> >> This doesn't make it at all clear *why* this is being done. What >> goes wrong if that doesn't happen? Where (as in, specifically, which bits >> of code) is it important that this is flushed? Is it just important >> to reduce memory consumption, or does it have semantic effects too? > > My first attempt at this was just to return the gem list. This had the effect > of pushing the test run into many many minutes before killing my machine - > ran out of swap (it never finished). So performance and memory consumption.
It worries me a little that the non-caching version could run the system out of memory, but not enough to reject the patch or anything. > So I went spelunking and found the caching pattern, added it, problem gone. > > From my - extremely limited - understanding of this specific code here this > just has the effect of clearing the cache after each client run, which sounds > sane. *nod* It sounds like the semantics you want, and the existing semantics, match. >> Why are we using a per-thread cache? > > cos thats the existing pattern I found in the file that had sufficient > comments > so their use were clear :) > > Do you want comments added to these 3 bits of code? Seems pretty clear to me > already If you don't mind. Your answers are what I expected: you did understand the existing semantics, and they did what you wanted. Not much of a surprise, given you take a cautious and complete approach to the code you write. Having the confirmation there in writing is a good for "me in six months", and for anyone else who comes along. :) >> On the testing front, I don't see any testing in there that tries to >> exercise the thread semantics, or verifies the life-cycle >> assumptions, or that verifies correct cache management. Given the issues >> we had >> from distant changes breaking those recently, in the area of thread >> caching, it would be important to see those. > > yes, I wanted to make tests too. No I dont know how, I went looking through > the code > and recent commits (like yours in 68c106e3ef192d64eb5a1e8daa1e070774909728) > but found > that the code got changed without tests being written. > > Without examples I'd have to say this is beyond my understanding of puppet > internals > to test. Darn. Here I was hoping that you might solve my problem for me. ;) Seriously, though, on my list of urgent things to do here is to replace that temporary work-around in our master branch with a long term version that doesn't have the memory-holding properties, and has clearer semantics than the current thread-global caching. So, yeah, I have no clear idea either. The tests you show below look like a good start to me. Hopefully they will give us enough coverage that an "unrelated" change that breaks those semantics will show up somewhere useful. So, add that comment and I am happy to see this go in. Throw my reviewed-by on the change and you can go ahead and merge it into master. (I think you have the commit rights already, but if not I will chase Zach to get them for you.) Thanks for writing this, and I think it is a great thing to have added to the system. Daniel -- ⎋ Puppet Labs Developer – http://puppetlabs.com ✉ Daniel Pittman <dan...@puppetlabs.com> ✆ Contact me via gtalk, email, or phone: +1 (877) 575-9775 ♲ Made with 100 percent post-consumer electrons -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to puppet-dev@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.
