On 11/06/2014 12:34 AM, Nathan Scott wrote:
> 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. :)
I've taken another look at this. I see where my confusion came from. We
have 3 distinct classes of dictionaries, each treated differently.
During PMDA.run(), the PMDA class passes down to cpmda several dictionaries:
(1) self._metric_names (used by cpmda.pmns_refresh)
(2) self._metric_oneline (used by cpmda.pmid_oneline_refresh)
(2) self._metric_helptext (used by cpmda.pmid_longtext_refresh)
(2) self._indom_oneline (used by cpmda.indom_oneline_refresh)
(2) self._indom_helptext (used by cpmda.indom_longtext_refresh)
(3) self._indomtable (used by cpmda.pmda_dispatch)
(3) self._metrictable (used by cpmda.pmda_dispatch)
Dictionaries marked by (1) are passed down, used once when needed, then
discarded.
Dictionaries marked by (2) are passed down and kept, used when needed,
never discarded.
Dictionaries marked by (3) aren't passed down and kept. Instead the PMDA
class creates a string buffer for each dictionary and passes that down
to cpmda. This string buffer is used once and then discarded.
I was thinking all dictionaries were treated as being in class (1) (used
once then discarded). That's why I was passing down everything again in
PMDA.__refresh_metrics(). Since I don't need to pass down items again in
class (2), that simplifies PMDA.__refresh_metrics() to:
====
def __refresh_metrics(self):
# Call the user's function.
if self.__refresh_metrics_func:
self.__refresh_metrics_func()
if cpmda.get_need_refresh():
self.pmns_refresh()
cpmda.pmda_rehash(self._indomtable, self._metrictable)
====
We actually could go further here, if we can change the cpmda interface.
I'm not sure how set in stone it is. I'm not talking about a change to
the PMDA class interface here, just the cpmda interface. If we change
cpmda.pmda_dispatch() to take the indomtable and metrictable
dictionaries directly (instead of using string buffers), we could keep
them around for use later by pmda_rehash() - basically moving them to
class (2).
If we go that far, then all dictionaries are in class (2) (passed down
and kept) - except for _metric_names in class (1). If we decide to move
it to class (2), then all dictionaries are treated the same. At that
point, we don't need a wrapper around the user's refresh_metrics()
function in the PMDA class at all, since the cpmda already has all the
information it needs (pointers to all the dictionaries) and knows when
to do the refreshing.
> 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
I'm afraid you'll have decide on/help me figure out pcp subtleties...
Right now the user's refresh_metrics callback function gets called down
in cpmda at similar places as the mmv pmda calls mmv_reload_maybe().
I'll need help in figuring out if that is causing unnecessary
duplication. Assuming the user's refresh_metrics callback is smart
enough to not change anything when it doesn't need to, then cpmda
shouldn't really refresh anything either.
>>> ... 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.
We should probably just delete the whole '#else' code to avoid confusing
others in the future.
--
David Smith
dsmith@xxxxxxxxxx
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)
|