On Sat, Jun 12, 2010 at 9:40 PM, Luke Kanies <[email protected]> wrote:
> On Jun 11, 2010, at 4:10 PM, Markus Roberts wrote: > > Jesse and I are shooting for the minimal viable fix here, with the idea >> that >> a great deal of refactoring is needed but isn't appropriate at this time. >> The >> changes in this commit are: >> >> * Index the function-holding modules by environment >> >> * We need to know the "current environment" when we're defining a function >> so >> we can attach it to the proper module, and this information isn't >> dynamically >> available when user-defined functions are being created (we're being >> called by >> user written code that doesn't "know" about environments) so we cheat and >> stash the value in Puppet::Node::Environment >> > > Is there any attempt at thread-safety here? I couldn't see any, which kind > of concerns me. > No. This is not thread safe, which is not a change; none of the code involved was thread safe and this patch does not make it so. Don't we only need that value set during loading? The rest of the time the > env value should be available. That is, functions should only be loaded > within the context of an environment, so 'current' shouldn't be necessary > then, right? > The earlier version of the patch was written that way, and that refactor should ultimately be done, but it became increasingly complicated the deeper we got into it and (since the quasi-global Environment.current was needed in any case) we decided to accept it as technical debt, use it as the single (and far simpler) mechanism and move on for now. > > I also don't see how this code knows when a given function should be loaded > into the root module vs. the current env? > > * since we must do this anyway, it turns out to be cleaner & safer to do >> the >> same when we are evaluating a functon. This is the main change from the >> prior >> version of this patch. >> > > I missed this code somehow... What do you mean? That we decided that having a single mechanism (Environment.current) was cleaner & would be easier to eventually unwind than having two. This is basically the point I just mentioned above. * Add a special *root* environment for the built in functions, and extend > all > scopes with it. > > * Index the function characteristics (name, type, docstring, etc.) by > environment > > * Make the autoloader environment aware, so that it uses the modulepath for > the > specified environment rather than the default > > * Turn off caching of the modulepath since it potentially changes for each > node > Note that this is potentially expensive - the module search path is built > from the lib dir of all modules in that env, so building this path involves > looking in every module in the env. Removing the cache can hurt. It might > be a better idea to cache per environment. If it turns out to be a significant performance hit, certainly. As with your patch, this was a step along the path from "not working at all" to "working really well"; it gets us to "working, though perhaps not elegant or performant" but Jesse and I are by no means in love with the state that it leaves us in. -- Markus P.S. Personally, I think stripping out all pretense of handling multiple environments in a single process is the way to go, but I understand the concerns that make that an unpopular direction. ----------------------------------------------------------- The power of accurate observation is commonly called cynicism by those who have not got it. ~George Bernard Shaw ------------------------------------------------------------ -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
