Hi guys,
----- Original Message -----
> [...]
> I'm planning to merge it to master as soon as I've finished the last few
> review tasks
> (the events support in particular) - probably sometime tomorrow.
Mark is away for the week, and next week is QA week for 3.11.0, so I'll
attempt to fill his shoes and get this merge over the line before then.
> I'm still not terribly
> happy with the use of 'ambiguous sentinels' in lieu of the optional error
So, I tend to agree this is close to merge-able now. I've pushed some
fixes and cleanups onto my branch on git.pcp.io - see separate mail.
There are three^Wtwo concerns holding me up at this stage:
1. The pmExtendFetchGroup_event API cannot support highres timers as-is.
Either we need a second API (for highres events), or we need to have
one API with struct timespec for the "out_times" parameter (and do a
lower-res timers usec -> nsec conversion within). I think the latter
is a cleaner approach, gives one fetchgroup API for both event types.
It would be good to complete the event support by adding highres, and
hence verifying suitability that side of the API.
2. Sentinel values
I'm not so much against sentinels in the way Mark seemed to be, as
it looks kinda handy, but the choice of zero as a sentinel integer
value strikes me as questionable.
Zero is far and away the most common integer metric value:
$ pminfo -f | grep 'value 0$' | wc -l
16121
$ pminfo -f | grep 'value 1$' | wc -l
860
... so why was that picked as the sentinel value? (esp. since NaN
is used for float/double, something uncommon for integers too would
seem better suited).
$ pminfo -f | grep 'value -1$' | wc -l
1
I recommend a "memset(&atom, -1, sizeof(pmAtomValue));" for integer
PM_TYPE_* variants, as that's very uncommon (i.e. all bits set). It
turns out that also gives a convenient NaN (see below, may help with
that other issue too).
3. pmclient_fg (binary and python variants) lacks QA, man page, and
there are a few problems in the pmclient makefile now.
[ I've fixed these, see separate mail ]
Minor issues, less pressing than the above (can wait post-merge):
- The save/restore of the context around every fetchgroup call seems
excessive (extra libpcp lock/unlock x2 for every call) and makes for
many "goto" statements in the code that would otherwise collapse to
much simpler error handling. This all seems a bit unnecessary - what
is the aim there? (and is it worth it?, it penalizes every client)
In one case (timestamp API) there were even no PMAPI calls but still
the context save/restore.
[ I've taken the latter out, and fixed some unnecessary use of goto,
but I think all of this context get/reset code on entry/exit can be
safely removed. ]
- The uses of nan/nand is going to cause problems on some platforms - we
can solve these the same way pmie and other places have though (it's
not enough to set _XOPEN_SOURCE, some platforms don't have, eg nand at
all, so libpcp cannot rely on that).
[ I'll fix this, will need check-builds on several platforms, depends
a bit on use of memset/not from above - will wait on that verdict. ]
- LLONG_MAX, LLONG_MIN, ULLONG_MAX - see config.h.in which has some of
these constants already defined.
[ I've done a partial fix here, but may be worth revisiting to remove
the fetchgroup #defines entirely ]
BTW, the code-style consistency has improved out of sight, thanks.
There's still a few inconsistencies I noticed - in general we try to
keep to 80 columns or fewer in core PCP code and have conventions around
comment styles too that were sometimes missed here. There's also some
man page formatting conventions we use throughout PCP that differed a
fair bit here. All trivial stuff though, I've tidied up all of those
aspects I think.
cheers.
--
Nathan
|