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
