[rrd-developers] [FORGED] r2130 comments : rrd_parsetime, create_set_no_overwrite

[kevin brintnall]

>
> >Also, in handle_request_create(), maybe it would make sense to do the
> validation (i.e. step and last_up) in rrd_create_r() and rely on the
> return value from rrd_create_r() to detect whether there was a failure?
>  That way, the validation code stays in the same place.
>

> The validation is normally done in rrd_create and so this is bypassed by a
> call to rrd_create_r; since it was two small checks I didn’t think it worth
> the effort of moving the tests from rrd_create to rrd_create_r (and again, I
> was trying to avoid modifying the rrd_create_r function).  I think this can
> probably stay?
>
>
I think this argues for moving the validation code to rrd_create_r or
rrd_create_fn, (depending on how the no-overwrite flag changes the API).
 Then, it can exist in a single place.  I don't see any external effects
there.


>
>
> >Also, on rrd_daemon.c lines 1830..1831, it looks like it could be
> re-written more nicely as:
>
> - status = buffer_get_field(&buffer, &buffer_size, &tok );
>
> - for(;(tok && !status);status = buffer_get_field(&buffer, &buffer_size,
> &tok )) {
>
> + while ((status = buffer_get_field(&buffer, &buffer_size, &tok) == 0 &&
> tok) {
>
>
>
> This is just a legacy of how this code evolved.  I’m happy to change this
> or not (you want another close bracket before the == though I think)
>

Yep, need the ). I typed the new line free-hand :)


>  In rrd_last.c, we seem to have lost a call to free(opt_daemon).  Is that
> intentional?
>
>
>
> Can you be more specific?  I’m not sure where you’re referring to.  The
> free() is still called in the argument processing; none of the other
> functions free the opt_daemon after handling either since it is potentially
> used later.
>

This line was removed from rrd_info.c and rrd_last.c:

     if (opt_daemon) free (opt_daemon);


>
>
> In handle_request_last(), it seems we could get the last update time
> without a flush by using cache_item_t.last_update_stamp and adjusting down
> to the nearest RRD step size?  Then, no flush is required and the existing
> behavior is preserved.
>
>
>
> Probably…  However I’m not familiar with the cache data structures and
> thought it best to avoid this, particularly since the previous behaviour did
> a flush anyway.  Also, by making the flush an optional parameter (in the
> client) it means you can retrieve both the last cache update time and also
> the last flush time.  So, the call from rrd_client will preserve the
> existing behaviour (return last data update) as it forces a flush.
>

>
> If we can get the last time accurately via the cache data structures then
> this would be preferable (although the rrd_info would still need a flush as
> it gets a whole stack more data as well) – can you let me know the code
> required to obtain it?
>
>
The cache structure maintains this value so that it can deny updates from
the past (just like the normal RRD code does).  So, to calculate the most
recent time stamp, it should be sufficient to do something like this
(paraphrased):

time_t t;
rrd_open(file); // bail if it doesn't exist
time_t from_file = rrd->live_head->last_up;
time_t step = rrd->stat_head->pdp_step;
rrd_close(file);
pthread_mutex_lock(&cache_lock);
ci = g_tree_lookup(cache_tree, file);
if (ci)
    t = ci->last_update_stamp;
else
    t = from_file;
pthread_mutex_unlock(&cache_lock);
t -= t % step;
free things;
return t;

The reason for this order is that you have to hold the cache_lock while
processing the tree.  We don't want to block out all other clients waiting
for the disk read to come back.  Note also, that we return the raw info in
case the file exists on disk but not in the RRD tree (i.e. hasn't been
written recently).

In the future if we keep step information in the cache structure (i.e. for
UPDATEV), we won't have to hit the file for the step..

Not sure how to handle the case of a version < 3 RRD file that uses
legacy_last_up.   Apparently rrd_last_r doesn't handle that today either.

-kb


>
> Steve
>
>
>
>
>  ------------------------------
>
> *Steve Shipway*
>
> ITS Unix Services Design Lead
>
> University of Auckland, New Zealand
>
> Floor 1, 58 Symonds Street, Auckland
>
> *Phone: +64 (0)9 3737599 ext 86487*
>
> *DDI: +64 (0)9 924 6487*
>
> *Mobile: +64 (0)21 753 189*
>
> *Email: s.shipway at auckland.ac.nz*
>
> P Please consider the environment before printing this e-mail
>
> * *
>
>
>



-- 
 kevin brintnall =~ /kbrint at rufus.net/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.oetiker.ch/pipermail/rrd-developers/attachments/20100924/a8d394ed/attachment-0001.htm