On Tue, 2010-12-07 at 20:42 +1100, nathans@xxxxxxxxxx wrote:
Helpful and constructive feedback as usual, thanks Nathan.
> 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?
Yep, e60cca18e5ef4afcff55762fb1cad9f6c73c5e46 is self-contained and
addresses a real (if low probability of occurrence) bug.
68492c0b96bfc41dc996c44583c7b50468a59299 is in a similar class ... real
bug, low probability of being seen in the wild outside QA ... but this
change is Not Quite Right(tm) as I just discovered, so skip this one for
the moment ... 8^)>.
> Random review comments follow, if unclear lemme know. :)
>
> - Is -x flag necessary for pminfo? (-f/p_value seems enough?)
I think is it needed. For event records, -f gives summary only, -x
gives details of each event record, e.g.
$ pminfo -a ~/src/pcpqa/src-oss/eventrec -f sample.event.records
sample.event.records
value [5 event records (7 missed) timestamps 15:26:00.073...15:26:04.073]
$ pminfo -a ~/src/pcpqa/src-oss/eventrec -x sample.event.records
sample.event.records
value [5 event records (7 missed) timestamps 15:26:00.073...15:26:04.073]
--- event record [0] timestamp 15:26:00.073 ---
sample.event.type (29.0.127)
value 4
sample.event.param_u64 (29.0.131)
value 5
sample.event.param_string (29.0.134)
value "6"
--- event record [1] timestamp 15:26:01.073 ---
sample.event.type (29.0.127)
value 7
sample.event.param_double (29.0.133)
value 8
sample.event.param_double (29.0.133)
value -9
--- event record [2] timestamp 15:26:02.073 flags 0x2 ---
sample.event.type (29.0.127)
value 10
sample.event.param_u64 (29.0.131)
value 11
sample.event.param_string (29.0.134)
value "twelve"
sample.event.param_string (29.0.134)
value "thirteen"
sample.event.param_32 (29.0.128)
value -14
sample.event.param_u32 (29.0.129)
value 15
--- event record [3] timestamp 15:26:03.073 flags 0x80000000 ---
==> 7 missed event records
--- event record [4] timestamp 15:26:04.073 ---
sample.event.type (29.0.127)
value 16
sample.event.param_float (29.0.132)
value -17
sample.event.param_aggregate (29.0.135)
value 18410503204342530817 -1.371380375612005e+306 [0103070f1f3f7fff]
> - 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?
Yep, missed that ... I'll review the man page and fix.
> - __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.
OK ... certainly winds the obfuscation dial right back in these code
blocks!
> - "if (flags == PM_ER_FLAG_MISSED)" ...
> a/ future-proof via "(flags & PM_ER_FLAG_MISSED)"?
> (several places, several different files)
No this one needs to be ==. if MISSED is set, then no other data can be
present in this event record, other than er_nparams which is the number
of missed records, so no other flag values should be present. I'll
tighten the logic in __pmCheckEventRecords and __pmDumpEventRecords to
check for _only_ MISSED being set.
If a PMDA has both events and missed events, I'm expecting one event
record for each real event and one event record for each batch of missed
events.
I guess in the other places I should change to & MISSED on the
assumption that if MISSED is set then any other flag values also set
should be ignored ... with the tightened check in __pmCheckEventRecords
that should be OK. The places where something could go wrong here are
in the byte swabbing routines and pmPrintValue where if MISSED is set, I
assume it is a missed records type of event record and ignore any other
flag bits ... if the event array format is bad, this could be ugly but I
don't think there is any real choice. In all other places
__pmCheckEventRecords is called beforehand, so that will catch an
expected flags setting.
> 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)
Yes I struggled with this one ... I think PM_EVENT_FLAG_MISSED is
probably a better balance between shortness of name and clarity ... I'll
make that change throughout.
> - sentinal vs sentinel spelling? (libpcp/src/events.c)
Spelling never was my long suit.
> - 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?)
Logic is correct ... see comments above.
> - pmstore event type check may as well be before the pmFetch since
> its just tests the descriptor type field from pmLookupDesc...?
Yep, moved check to just after pmLookupDesc() call succeeds case.
> - 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)
Agreed (moving to PMDA_INTERFACE_5 removes the need for holding and
releasing the string in the callback)... I have a patch for this, but to
test it I'm struggling to find a PMDA that uses Perl and exports string
values and I can easily get the PMDA working locally ... mysql seems the
most likely, any other suggestions?
|