Hi David,
----- Original Message -----
> [...]
> As for the JSON PMDA itself, I'd like you (or anyone else who is
> interested) take a look at it and see if there is anything else needed
> to do to it before it gets merged.
I think its looking really good now. Bunch of small stuff from review
follows, then I think its fully cooked...
- you will see a merge conflict on pcp.spec.in against master (just a
heads-up - easy to resolve) from pcp-shping changes landing alongside
yours
- qa/1052 seems to switch off pmlogger - it shouldn't (this is required
by qa for other tests), what was the problem there? good test though.
- testing of the Install/Remove scripts is missing. Since we have 1052
doing detailed pmda testing, just something bog-simple like qa/755 (with
an effective s/apache/json/g) would be appropriate
- testing of the generate_ceph_metadata script is missing - can we give
that script a mode where it reads from a file, perhaps? (and feed it a
known good chunk of ceph output - since setting up a ceph cluster is ah
outside of the scope here). We need something there though, to give us
python2 vs python3 confidence for this script (as well as it being good
to have a basic test for it anyway)
- extend qa/src/torture_cache.c to cover exercising pmdaCacheResize and
qa/201 extensions as-needed to exercise the new libpcp_pmda API
- libpcp_pmda code is spot-on! very nice work, thanks David - there's
other parts of PCP that are now able to improve too, in future releases,
thanks to the work you've done here.
- copyright/gpl notice missing at the head of pmdajson.python and also
generate_ceph_metadata
- some copyright notices say 2014 - should probably be updated to either
just 2015 or 2014-2015 ... whichever is appropriate.
Thanks again David. There's one or two other areas we could continue to
progress this concept post-merge, if you're interested. But, don't want
to get side-tracked by those at this stage - lets get it merged and then
chat further if keen.
cheers.
--
Nathan
|