Hi Paul!
----- Original Message -----
> Hi Nathan,
>
> It has taken a bit longer than I originally planned it to but I have
No problem.
> made all of the suggested changes to the code that were pointed out.
Thanks, its looking really good.
> These changes are available at git://github.com/pauljevans/pcp.git gfs2
>
> I look forward to feedback.
I've pulled in your latest changes, and re-reviewed. A few things here
and there stood out in the new code - where possible I've fixed things
up that seemed NQR. A couple of questions/comments remain - see below.
On an unrelated note, I've pushed our changes to the gfs2 branch on oss.
I noticed your last commit message was extensive (which is good!) but it
had no line wrap (less good) - so, I did a git rebase then a "git commit
--amend", and then ran your message through fmt(1). *NOTE* that this'll
mean a git pull on your end will not work now. Not a problem - if you
just re-clone or create a new branch based on the new gfs2 branch, all
will be well for your next set of changes/updates.
This is now looking almost ready for merging. Please look over the few
changes I made (see git commit messages in separate mail) - I hope they
are self-explanatory and make sense, if not please lemme know.
OK, so those last couple of things ...
- I didn't really understand the intention of the on-stack "value" local
variable in gfs2_store(). Its initially set to zero, never modified,
but is tested for equality to zero or one later on...? Is this just a
leftover from earlier development, or some other intention there?
- The QA tests both fail for me. The way the testing model works is a
test driver script (qa/check) runs the test(s) and then compares the
output they produce to the expected output (i.e. 654.out & 655.out).
The expected output file for 655 is missing, and 654 doesn't produce
the output in 654.out anymore.
More information on the testing methodology can be found in qa/README in
the git tree.
Once the above are resolved, it is looking ready for merging to me (which
will see it arrive in pcp-3.8.1 at this stage).
The next steps from there would be:
- A man page for pmdagfs2 would be a good idea, esp. since its a fairly
complex agent - see man/man1/pmdacisco.1 as an example. Description of
the store-to-enable/disable mechanism would be helpful.
- A client tool to drive the extraction and reporting of stats from the
various cluster nodes, manage the enabling / disabling of tracing, etc.
As discussed, python might be an appropriate language (other option is
C), some tests and a man page for the new tool would be a good idea as
well. Looking forward to this - should be an interesting bit of code!
cheers.
--
Nathan
|