----- "Ken McDonell" <kenj@xxxxxxxxxxxxxxxx> wrote:
> Bit of a mixture here ... more event records refinement, anonymous
Looks good. Few review comments follow.
> metrics plus some real bug fixes. Most contentious changes are
> probably in pmns.c and pmdaproc.sh
No issues here with any of those. WRT the "real bug fixes", are they
self-contained? (e60cca18e5ef4afcff55762fb1cad9f6c73c5e46, at least,
looks like a candidate for merging to dev for next bugfix release).
Are there others?
Random review comments follow, if unclear lemme know. :)
- Is -x flag necessary for pminfo? (-f/p_value seems enough?)
- pmUnpackEventRecords man page has an nmissed parameter which
no longer matches code ... perhaps this man page should cover
event.missed and event.flags a bit instead now?
- __htonpmValueBlock and __ntohpmValueBlock - given the relative
complexity of swabbing PM_TYPE_EVENT, that (new) code would be
cleaner in separate static routines (more readable) - esp. in
the second case, with the switch, closing braces offsetting is
a bit strange.
- "if (flags == PM_ER_FLAG_MISSED)" ...
a/ future-proof via "(flags & PM_ER_FLAG_MISSED)"?
(several places, several different files)
b/ PM_ER_FLAG_MISSED -> better name? (er ~= error? emergency
room? errr - not sure?)
(PM_EVENT_FLAG_MISSED? PM_EVREC_FLAG_MISSED? or spell out
in full even PM_EVENT_RECORD_FLAG_MISSED)
- sentinal vs sentinel spelling? (libpcp/src/events.c)
- do_events in pmlogger/events.c - if MISSED flag is set, rest of
event record array interpretation is skipped - is that right?
what if we got some and missed some...? (shouldn't processing
continue?)
- pmstore event type check may as well be before the pmFetch since
its just tests the descriptor type field from pmLookupDesc...?
- Perl PCP::PMDA module should be updated to improve its string
handling on fetch callback ... think it jumps through some
hoops that are no longer needed with appropriate return codes?
(see saved_string in PMDA.xs)
cheers.
--
Nathan
|