On 05/22/2015 02:47 AM, Nathan Scott wrote:
> 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
Fixed.
> - 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.
I must have gotten that pmlogger change from whatever test I started
with. The test no longer switches off pmlogger (and runs just fine).
> - 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
New test qa/1053 does install/remove testing.
> - 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)
I added basic generate_ceph_metadata testing to the qa/1052 test. As far
as python2 vs. python3 goes, I made several changes to both the pmda and
the generate_ceph_metadata script to handle those issues.
> - extend qa/src/torture_cache.c to cover exercising pmdaCacheResize and
> qa/201 extensions as-needed to exercise the new libpcp_pmda API
Done.
> - 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
Copyright/gpl notices added.
> - some copyright notices say 2014 - should probably be updated to either
> just 2015 or 2014-2015 ... whichever is appropriate.
Fixed.
> 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.
Sure, I'd be happy to talk more after the merge.
--
David Smith
dsmith@xxxxxxxxxx
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)
|