Theo Van Dinter writes: > On Sat, Feb 25, 2006 at 12:52:16AM -0800, Loren Wilton wrote: > > Without looking at the code, and as something of an outsider, some thoughts > > that may be useful: > > :) > > > 1. Is there any notable rule efficiency difference between being an eval > > and being a plugin? > > The results are exactly the same. > > > 2. If there isn't any notable performance difference, then this amounts to > > an interface change. Assuming the evals use standard plugin interfaces > > (assuming plugin interfaces ARE standard), then fewer interfaces is probably > > a good thing. If the evals require a bunch of hackish special interfaces to > > work, maybe it is a bad idea. > > There's nothing hackish involved, but I don't have performance numbers.
If anything, it's easier since, as Theo said, it means that there's one less wierd legacy interface left in the code; the idea of the EvalTests.pm file inserting methods directly into the PerMsgStatus namespace is exceedingly hacky. > > 4. Not splitting evals into separate rules: what effect would this have on > > trying to priortize and short-circuit rules? Would it be possible to run > > some rules in a package and not others? > > None, and yes. Having the code as plugins instead of in EvalTests just moves > where the code lives, and allows for code to be disabled or swapped out. > > > 5. It would probably be good to consider the idea of a "welded plugin" that > > can't be unplugged and doesn't need to be explicitly plugged in someplace. > > I think that's pretty easily doable (add load_plugin() calls to > M::SA::init()) if we wanted to go that route. For eval rules, it would > at least let us have a standard API, and clean up the PerMsgStatus object, > though I like the idea of having plugins be unpluggable. :) I don't think there's a need for any EvalTests to *always* be loaded. Bear in mind that the EvalTests functions are already not guaranteed to be run, since right now a user can set their scores to 0 and they won't run. Plus I like the idea of not even loading the DNS code if -L switch is being used; it'd save memory! The only thing I can see that could be problematic, is how many new plugin modules it creates. If there's more than 3 or so, I'd prefer to see it around 3. This really ties into John's concerns. in my opinion having a load of small plugin files isn't really a win unless there's real reasons to have eval-test function A be in a different file from eval-test function B (such as the network-test example). Overall though: +1 on this work, and +1 on merging this to trunk if all tests pass with "make test" and "make disttest". --j.
