Ken McDonell wrote:
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.
All your currently proposed options are small constant strings, so
there's no difference between the models in terms of type safety.
Separate functions would allow the types of options to vary in the
future without compromising type safety. And the real reason for
ioctl() style interfaces - the difficulty of adding new system calls or
likewise extending a fixed binary ABI - doesn't apply in this case.
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.
Ok, sounds consistent and workable.
I think it would be a good idea to carefully define and document the
semantics of calling pmiChangeStream() while partway through a sequence
of pmiPutValue() calls, i.e. what happens to the partly constructed
pmResult.
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.
Righto.
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, ...).
Sure, but all that state is in the C library right?
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.
Works for me.
--
Greg.
|