Thanks for the feedback Greg.
Comments in situ below.
On Fri, 2010-07-02 at 12:49 +1000, Greg Banks wrote:
> ...
> > int pmiSetOption(int option, char *value)
> > Call this early on to set archive name (option == PMI_ARCHIVE),
> > archive's hostname (option == PMI_HOSTNAME), archive's timezone
> > (option == PMI_TIMEZONE), etc.
> >
> Why not have separate pmiSetArchive(), pmiSetHostname() et al?
I was not sure how many options might be needed ... and I'm old enough
and scarred enough to believe the ioctl() way is not all bad ... 8^)>
I think this is a religious issue ... the alternatives are the same in
terms of ease of use and functionality ... and I'm not passionate enough
to defend either approach, so I'll follow the votes of the majority.
> > int pmiAddMetric(char *name, pmID pmid, int type, pmInDom indom,
> > int sem, pmUnits units)
> >
> I know it's against the PCP tradition to be const correct, but could we
> perhaps?
PCP pre-dates many modern C constructs ... but yes, you're correct and
I'll fix this.
> > int pmiGetHdl(char *name, char *instance)
> > int pmiPutValueHdl(int handle, pmAtomValue *atom)
> >
> Good idea, but I don't see why you can't call these *Handle. Vowels are
> cheap.
OK.
> One other idea: if the timestamp argument to pmiWrite() is NULL, you
> could internally substitute the result of gettimeofday().
Agreed ... this reflects the bias I have in coming at this from the
perspective of an existing repository of historical data being imported
and converted to a PCP archive. For real-time use, your suggestion
makes sense and I'll include that option.
>
> I really like the PutValue{*}/Write design, this should definitely make
> life easier for the filter writer. But what happens if you need to
> parse a data file which you want to split into 2 or more PCP archives
> (say because it contains data from multiple hosts interleaved) ?
OK that's outside the design centre I was considering, but I agree is
not unreasonable in some cases.
For this to work, I'd need to add a pmiStart() routine that would return
a stream id and set the current output stream. So for the single stream
case, nothing much changes. Adding a pmiChangeStream(int stream_id)
method would allow the multi output stream use.
This would be very similar to the pmNewContext and pmUseContext model
used for a PMAPI client connecting to multiple metric sources.
>
> Also, what's the defined behaviour w.r.t. threads?
I was not planning anything different to the underlying libpcp that I'll
be using ... so calling into libpcp_import from more than one thread
will lead to undefined outcomes.
> > For each of the routines above there would be a Perl wrapper
> > (pmiFooBar()
> > is wrapped by foo_bar()) and some constant functions and helper
> > routines,
> > so a Perl pmimport program might look like:
> >
> > use PCP::IMPORT;
> >
> > use vars qw( $import $myindom $inst $ctr $now $time );
> > $import = PCP::IMPORT->new('');
> >
> I don't see any point to having this object; if I understand the API
> above it will have no state and just serves as a subroutine scoping
> mechanism.
There is a heap of state that has to be held across calls to
libpcp_import routines (metadata, partial pmResult, ...). But the use
of an object framework is simply copied from PCP::PMDA ... remember I
know nothing about Perl, so I'll happily take advice on this.
> >
> >
> > End of archive processing is needed to write the log trailer and is
> > handled by an atexit handler.
> >
> I'm not impressed by that. I think you'd be better off with an explicit
> close function. I think that to be truly useful the API should be
> capable of being used in a long-running program which writes multiple
> archives concurrently or serially at it's whim.
Yep. If there is a pmiStart() routine, there should be a pmiEnd()
routine.
|