Hi guys,
Just had a wander through current status of all your work
in git://sourceware.org/git/pcpfans.git fche/dev branch
(which contains Dave's too! yay, too easy).
I made a series of notes - a braindump follows, the files
were visited in no particular order so its a bit random.
Anyhow, its all looking pretty good to me and I'm looking
forward to trying it out. I'd really like other folks to
take a look too & offer their own review commentary also,
should they have time/inclination - thanks!
cheers.
ps: I'm away for 3 weeks, starting .... now. See you next
year, I hope you have a safe and enjoyable holiday season!
--
Nathan
[pmmgr man page]
No config comments allowed? Just README - someplace else though?
seems a bit harsh. *shrug* - good reasons for the rigidity I hope.
Man page style - command names usually in bold in PCP man pages.
A BUGS section? g'wan, we don wan no steenkin bugs!
[build/makefiles]
CXXMDTARGET - hah! sneaky choice of name.
pmmgr not on windows? (pmcd+pmdas & libpcp shows how it can be done)
- this would be really good to have there, no cron on Windows (totally
different mechanisms provided by OS, of course) - so PCP missing all
of this functionality on Windows atm.
- having looked at the code, yeah, I see why now. still, if we can
keep together / abstract out those bits which are known to be deeply
platform (posix) specific then porting becomes much easier. mostly,
it is that way (eventually, separate files would help) - good stuff.
[libpcp]
pmDiscoverServices -> numUrls += ... (else, no point assigning to zero)
internal.h - no need for new #ifdef there.
pmDiscoverServices needs a man page & automated QA (via pmfind perhaps).
[pmfind]
Default action is to do nothing? (make -p the default?, ie default
to pmcd service but allow cmdline option for others?)
Needs a man page & automated QA.
[pmmgr]
log-directory config file hard-codes /var/log/pcp ... use $PCP_LOG_DIR?
(can these config files have embedded env vars?)
target-discovery.avahi contains avahi? (latter seems superflous)
misspelt pmwebd reference in rc_pmmgr still (cut&paste - patch attached)
makefile - targets (not macros) need platform protection via ifneq -
e.g. see each of the platform PMDAs makefiles, and "build-me" (so, don't
build/install at all if not supported, but CFILES/HFILES/LSRCFILES still
needs to be set)
(remove MINGW refs in code - makefile guarantees this for us, until
someone does a port - so its just distracting noise in there too)
Log merging - via pmmgr_pmlogger_daemon::daemon_command_line side-effects?
This part feels a bit hacky - wonder if these helper programs could be
more "designed in", somehow, perhaps with their own helper interfaces
(that routine has got pretty big... and has all sorts of extra goodies
unrelated to daemon command lines).
Log file naming conventions - one of the issues with the current scheme
is that a host can end up with many, many archives in one directory, if
logs allowed to accumulate over years (which does happen). Consider an
alternate scheme where the filename is short, and directory components
are used - ie 20131213-xxx.* -> 2013/12/13-xxx.* (maybe as an option,
maybe by default ... needs thought/discussion ... I prefer the latter,
personally - tackles a known problem - others care less AIUI).
Log rewriting (pmlogrewrite) seems to be missing? (nor in the todo)
I like the host-id stuff - neat approach! :) I wonder if we should
take some steps (sort?) to attempt to ensure a given series of metrics
will always give the same host-id (for same values returned of course).
I was half expecting this to be all threaded - especially the bits that
restart each daemon & merge logfiles and so on - we'll block in those
spots, so one slow merge/reconfig/restart slows all, and daily logger
restart gets slower with more and more hosts...? *shrug*
[packaging]
What's the plan? If we want to allow people to pick and choose between
cron-based and pmmgr-based (which I imagine we do) we need to split both
styles into separate rpms which replace/conflict with each other, I think?
pcp-manager-static and pcp-manager-dynamic? (maybe Provide: pcp-manager
from both, and pcp could depend on that virtual package?)
Without this, we're stuck without a strategy for not needing to install
crontab files and so on. And if we don't tackle it -> pain later (get
to deal with conflicts on released pcp packages that contain pmmgr too).
The pmie and pmlogger rc scripts (and associated chkconfig state) are no
longer used in this scheme AIUI, right? (nice) If so, all of:
/etc/rc.d/init.d/pmlogger
/etc/cron.d/pcp-pmlogger
/etc/pcp/pmlogger/control
/etc/pcp/pmlogger
/usr/libexec/pcp/bin/pmlogger_check
/usr/libexec/pcp/bin/pmlogger_daily (?)
(and pmie equivalents) would move into pcp-manager-static, I think - all
with the nice side-effect of shrinking the size of a "core" pcp install
(for hosts not wanting this stuff, when monitored remotely).
Also, should plan out what happens when someone switches from one approach
to the other (and back again), cos its bound to happen whether by accident
or not - we need to have thought it through (test) and make sure its safe.
rc_pmmgr.patch
Description: Text Data
|