Hi David,
----- Original Message -----
> On 11/04/2014 11:40 PM, Nathan Scott wrote:
> > (for an API we are expecting PMDAs to use). If not explicitly
> > needed, we should keep it simple and drop it (but, maybe I am
> > missing something there?)
>
> You have missed something here. See below.
That's usually it :) - thanks for the extra explanation.
> > of "refresh" work from a PMDA is done). Can we set that "refresh
> > needed" flag in cpmda land from the PMDA and delay this work til
> > then? That'd also encapsulate a bit more of the implementation
> > details by pushing that down into the lower layers of the wrapper.
> >
> > + # Call the user's function.
> > + if self.__refresh_metrics_func:
> > + self.__refresh_metrics_func()
> > +
> > + # Now that the user's function has been called, we need to
> > + # refresh all our class internal data.
> > + #
> > + # FIXME: instead of using cpmda.get_need_refresh(), we could
> > + # ask the user to return a '1' from the refresh metrics
> > + # function if a refresh is needed.
> >
> > + if cpmda.get_need_refresh():
> > + self.pmns_refresh()
> > + cpmda.pmid_oneline_refresh(self._metric_oneline)
> > + cpmda.pmid_longtext_refresh(self._metric_helptext)
> > + cpmda.indom_oneline_refresh(self._indom_oneline)
> > + cpmda.indom_longtext_refresh(self._indom_helptext)
> > + cpmda.pmda_rehash(self._indomtable, self._metrictable)
> >
> > This ended up with lots of calls back down into the cpmda layer
> > from the higher level python code - kinda feels like we may have
> > some unnecessary up/down-calls here? (if that makes sense at all).
> > IOW, can we push most of this down into the cpmda layer, hopefully
> > making the interface exposed to PMDAs a little simpler/clearer?
>
> So here's the deal with this. I probably should have explained this
> better when I sent the patch.
>
> Right now we've got 3 levels:
>
> - low (cpmda)
> - medium (PMDA class)
> - high (user's class derived from the PMDA class)
>
> Up at the high level (the user's class), a user calls add_metric() (for
> instance), which updates several of the PMDA's class internal variables,
> then calls the cpmda as needed.
Yeah, but :) ... its a bit more convoluted than that isn't it? e.g.
the dictionary "self._metric_helptext" at the medium level is also
open for business down at the low level, via "pmid_longtext_dict" -
isn't it? If this is as I remember it, the boundaries are blurred a
bit here for convenient access between middle and lower levels - we
may be able to exploit that a bit to simplify the new API?
If its not as I remember, just disregard this & carry on. :)
The only other thing I wanted to revisit a bit is this subtlety...
> > I was expecting this "refreshing" would happen around the time
> > PDUs exchanges happen with pmcd, which delays it until the last
> > moment (and prevents the situation where unnecessary duplication
Its not 100% clear to me from...
> So, when a user wants to use the new refresh functionality, he calls
> PMDA.set_refresh_metrics(my_refresh_function). From the user's point of
> view, here's what happens. His 'my_refresh_function' gets called when
> necessary.
... what the definition of "when necessary" is? Is it at the time
we are responding to PDUs from pmcd, or is it the time the PMDA has
realised "something changed" (i.e. up at the high level)? Heh, it's
a twisty maze following all these callbacks - sorry.
If its the latter situation, we'll sometimes end up doing unnecessary
refreshing - so if that can be delayed until PDU processing time (eg
via that existing just-set-a-flag-and-refresh-me-later mechanism), that
will help reduce CPU time for some situations.
>
> If you don't like the PMDA class wrapper around the users' refresh
> function, another solution would be for the PMDA class to provide a
> function that would need to be called at the end of the user's refresh
> function to update everything (kind of like the PMDA.run() function).
>
I've not got a strong opinion really - stick with what you have? sounds
simpler.
>
> With the current code, calling this with an empty indom list would work
> fine. Calling it with 'None' would probably fail.
Okie doke, something worth double-checking anyway I guess (since "fail"
here means "generate an exception and exit(3) the PMDA process", a long
running daemon).
> > #ifdef PyBUF_SIMPLE
> > + static void
> > + pmda_refresh_metrics(void)
> >
> > ... you've got a fair bit of new code there that's only going to be
> > compiled for the python 2.x series (guarded by the above ifdef) - I
> > haven't tried, but I'm punting we'll need to refactor a bit here to
> > make this work for python 3 as well.
>
> Hmm, from my reading of the python devel code, PyBUF_SIMPLE is defined
> in python 2.6+. I think the code in the '#else' here is dead code for
> python < 2.6.
Ah, yes, right you are - I was thinking PyBUF_SIMPLE was removed in 3.x
but on closer inspection it is still there & yep, looks like dead code.
I do vaguely remember some py3 porting issues around buffers - but this
looks unrelated.
cheers.
--
Nathan
|