Re: Timed cookies

On Sun, Oct 21, 2007 at 03:52:54PM +0200, Krzysztof Oledzki wrote:
> >>New information are presentetd in two different ways:
> >> - for HTTP: a "human redable form", one of "100000d 23h", "23h 59m" or
> >> "59m 59s"
> >
> >Please compact the time as much as possible. Simply remove spaces. Also,
> >you can compact the number of days. I even think that we can report the
> >number of days in digital format. Eg: 12.5d.
>
> Right. If that is fine for you I would like to keep the
> 24855d23h/23h59m/59m59s format since it is hard to tell what does 12.3d
> exactly mean. IMHO 12d7h looks better than 12.3d if you need to check if
> it is what you expect.

OK, you're probably right.

> >OK, I had already fixed the number of columns as the first post-1.3.13
> >patch, but I did not notice that colspan was wrong.
>
> OK, I will try to extract this patch from the git repository to prevent
> conflicts.

It's commit d4e1b5... It's quite trivial, changed cols="20" to cols="22".

> >Use a long instead of a time_t for last_change, because it's
> >awkward to call time() every time you need it. There is already
> >a variable for this : "now". It is a timeval, so everywhere you
> >need the current time, you can simply use "now.tv_sec". It is
> >really cleaner IMHO because it is guaranteed not to change during
> >a "cycle". Also, it avoids many useless syscalls.
>
> Right, as always, I did not know that such thing exists. ;) Anyway, I am
> not sure about this time_t. AFAIK the new standard is to use time_t
> everywhere where possible since it will allow to switch from 32 to 64 bits
> in the future. I just checked, and on Linux, struct timeval defines
> tv_sec as time_t:
>
> struct timeval {
> time_t tv_sec; /* seconds */
> suseconds_t tv_usec; /* microseconds */
> };

Ah, I was not aware of this. Now that you say this, I have two different man pages :

  with old kernel and glibc 2.2.5 :

       struct timeval {
               long tv_sec;        /* seconds */
               long tv_usec;  /* microseconds */
       };

  with "new" kernel and glibc 2.3.3 :

       struct timeval {
               time_t         tv_sec;        /* seconds */
               suseconds_t    tv_usec;  /* microseconds */
       };

In any case, it looks like they're plain compatible, so feel free to work towards the future and use time_t :-)

> >>diff -Nur haproxy-1.3.13-orig/src/checks.c haproxy-1.3.13-sla/src/checks.c
> >>--- haproxy-1.3.13-orig/src/checks.c 2007-10-18 22:38:22.000000000 +0200
> >>+++ haproxy-1.3.13-sla/src/checks.c 2007-10-21 02:47:23.000000000 +0200
> >>@@ -57,48 +57,54 @@
> >> struct session *sess;
> >> int xferred;
> >>
> >>+ if (!(s->state & SRV_RUNNING))
> >>+ return; // already down, do nothing
> >
> >I would prefer not to rely on SRV_RUNNING inside checks.c. The reason is
> >that it is checks.c which is responsible for setting this flag for others.
> >In some sort, it is an output. All the checks algorithms relies on
> >s->health,
> >and I'd really like to keep that as the primary source of information.
>
> All I was trying to change here is to prevent doing something which was
> alredy done.

yes, that's what I understood too. But the check on health does it too as it only matches upon a transition.

> If SRV_RUNNING flag is cleared then there is no need to clear
> it again. I needed it to bypass updating last_change if nothing was
> changed.

OK, but even if it is set, there's almost no need to check it either. It must only be adjusted upon transitions. What happens is the following :

[server down] health == 0

    ...        health ++
    ...        (...)
    ...        health ++
    ...        health == rise-1   => server UP => health set to rise+fall-1
[server up]    health == rise+fall-1
    ...        health --
    ...        (...)
    ...        health --
    ...        health == rise  => server down => health set to 0

I don't know if I explain clearly, but I think you'll get it. The idea is just to create an hysteresis.

> >(...)
> >
> >>- if (s->health == s->rise) {
> >>- recount_servers(s->proxy);
> >>- s->proxy->map_state |= PR_MAP_RECALC;
> >>-
> >
> >Also, BTW, the patch is wrong, because s->health can only equal s->rise
> >when the server is *going* down. So we enter the if only during the
> >transition. With the change, we enter it every time a server is UP (=
> >mostly everytime). If you really want to remove this "if" block, you
> >can simply revert the condition and return early :
>
> Not sure about this as set_server_down is only called whet the server is
> going down. So, IMHO it is pointles to check if it is going down if it
> _is_ going down. Additionally, if I made this wrong last_change would be
> updated everytime and it is not what my test shows.

Looking at the code, it looks you're right. Probably that we can simply get rid of the checks completely. This would make the code cleaner and "easier" to understand.

> >
> >> if (s->health >= s->rise) {
> >>- s->state |= SRV_RUNNING;
> >>
> >>- if (s->health == s->rise) {
> >>+ if (!(s->state & SRV_RUNNING)) {
> >
> >same as above about the transition vs state.
>
> OK, if you do not like my changes I can move the place where the
> last_change is updated. However, I thought it is quite intuitive to do it
> in the same place where s->state is changed, which is not possible
> currently as s->state is overwritten every time.

I agree to place it at the same place that s->state is changed. I just don't like relying on a level while earlier it relied on an edge. It seems to me that it's not required to change the test.

> >>+char *human_time(unsigned t, int hz) {
> >>+ static char rv[sizeof(" 100000d 23h")+1]; // longest of " 23h
> >>59m" and " 59m 59s"
> >
> >you cannot reach 100000 days, so you can strip one char here. However,
> >I don't know what happens with negative time. This would be interesting
> >to check (you see, when people brutaly set the system's time by hand in
> >the past).
>
> Thank you for pointing this. I will fix this.

This does not necessarily need a fix. Maybe a quick check is enough (with the appropriate comment in order to stop us from wondering next time).

> >BTW, I generally do not change people's patches before committing
> >them. Some contributors may find it insulting that I fix things in
> >their back, and others get annoyed because they maintain incremental
> >patches on their side.
>
> That is true. However, I have no objection when you fix something trivial,
> obviously and wrong. Especially that it is _your_ project and there is no
> way to force you to accept something you do not want to.

But if I refuse everything, I'll never get any contribution :-) Aleks Lazic and Arnaud Cornet got used to my borring reviews and fortinately did not get discouraged.

> On the other
> hand, it is nice to know what was changed, for example to be in sync with
> the mainline.

Exactly. And also because sometimes I may be wrong and it's good that you can double-check what I commit.

> >On the other hand, I know it's a waste of time to play ping-pong
> >with patches just to fix minor issues, hence why I've recently
> >slightly modified some patches I was merging. Depending on what
> >you expect when sending a patch, do not hesitate to tell me when
> >you're OK with me doing minor changes if I notice anything while
> >merging, to ask for review when it is what you're waiting for,
> >or to explicitly tell me that you want a commit or reject but no
> >change, for instance because you have other changes pending, or
> >because you want to discuss the reason for changes before the
> >commit. I have no problem adapting to people's expectations, we
> >just have to be in sync.
>
> Completely fair for me.

Perfect!

Cheers,
Willy Received on 2007/10/21 16:26