Hi Patrick,

On Thu, Aug 09, 2018 at 06:29:33PM -0400, Patrick Hemmer wrote:
> I also went and removed the queue position counter code from
> stream_process_counters(), and the logging still appears to work fine
> (but I could have easily missed some potential scenarios).

OK nice!

> In regards to the idea you added to the commit message on the queue index
> > It could even be further improved to avoid manipulating the stream
> > from the queue :
> >   - queue_idx = initial position when enqueuing
> >   - queue_idx = measured position when dequeuing
> 
> Because the stream can pass through multiple queues, we'd have to make
> sure that every time we de-queue, that the stream code pulls the value
> and increments itself. However looking at all the various places
> pendconn_unlink() gets called, I think it would be difficult for the
> stream code to know when the value needs to be pulled.

In fact the only two ways to be queued multiple times are :
  - get redispatched after a failed connection attempt to a server ; in
    this case the pendconn was already dequeued to connect to the first
    server so the state is unambigous

  - being redistributed when a pendconn was lying in a server's queue
    and the server is marked down. In this case the pendconn is simply
    unlinked, process_stream is woken up and I don't know if we collect
    queue_idx during the call to process_stream before queuing again.

Anyway this is very minor, I'd even say cosmetic. I'm pretty fine with
the current state already!

> Anyway, attached is the latest patch set for review again.

I've applied it to a local temporary branch and am willing to merge it
if you're OK. I've only adjusted 3 minor things :
  - removed the warning in the 3rd patch's commit message mentioning
    the missing calls to pendconn_unlink() before logs, because these
    ones were finally moved to patch 1 but we didn't edit this message ;

  - removed the now unused queue_idx from stream_process_counters()
    which emits a warning since it's unused

  - removed the "__decl_hathreads(HA_SPINLOCK_T lock);" from struct
    pendconn that is a leftover from the rebase onto the last queue
    updates.

So please just let me know, I'm ready ;-)

Thanks,
Willy

Reply via email to