Hi guys,
----- Original Message -----
> > The pcpfans.git fche/pmwebd contains a reviewworthy state of the
> > pmwebd+graphite code. It's just been rebased based on /dev, and
> > contains a bit of a mishmash of commits (I could squash them into one
> > if y'all so desire). All the new code is under src/pmwebapi, which
> > was reworked into C++ and much enhanced. Man pages have been updated.
> > The /etc/pcp/pmwebd/pmwebd.options file has a more standard & more
> > useful format.
> > The qa/660 testsuite has been extended based upon a
> > gcov (test/code-coverage) analysis of the source code, so that
> > apprx. only trivial or hard-to-trigger error cases are left untouched.
> >
> I finally made time for my much-promised review of this work. I first
> familiarized myself with the existing src/pmwebapi code on the current
> dev branch. With that as a basis for review I looked at the new code on
> your branch. What I see there is a translation of the old to code to C++
> plus the addition of support for graphite.
>
> The addition of the graphite support is a straight-forward extension of
> the existing pmwebapi support and there is really nothing to comment on
> there. It looks like it was done in a way consistent with the existing
> support.
Excellent.
> So, I turned my attention toward the C++ re-implementation, looking for
> potential problems in the translation, resource leaks and the like.
> Despite my best efforts, I was only able to identify one potential
> problem in the function pmwebapi_gc (). The loop relies on iterators of
> a map remaining valid while deleting elements from the map. However,
> even this turns out not to be a problem based on my reading of
> http://www.cplusplus.com/reference/map/map/erase/ (see Iterator Validity).
Thanks for the detailed review Dave, great stuff!
FWIW, other things I'd be looking for when I'm reviewing would be :-
are new tests needed or is extension fine (long tests get difficult
to diagnose -> see Kens recent mail, end of last release), are other
existing tests still likely to pass (ideally, run things in your own
regularly-scheduled QA runs - git is massively helpful there), are
there packaging issues, compatibility issues, is there overlap with
other code in PCP (reuse potential) - that kind of stuff. Most of
those are covered off here, thanks so much!
I just noticed mention above of "more useful [config file] format" -
set an alarm bell ringing - is that a backwards-compatible change?
(and which standard is referred to - the usual pcp .options format?
was it not like that before? sorry, been awhile since I looked at
this code)
> So, congratulations! I am unable to find any issues with this work!
Indeed, that's a fantastic effort - not an easy undertaking with so
little initial clarity around how to approach PCP web interfaces and
this is clearly a huge step forward.
Sounds like Franks code/docs/qa is all good (assuming no compat issues
there?) - we just need to sort out the remaining issues around all the
javascript code/images from other projects & packaging thereof. I'll
continue that discussion in the other thread though.
Thanks again for reviewing - please do more of 'em, anytime! ;)
cheers.
--
Nathan
|