On 2018/8/10 09:19, Willy Tarreau wrote:
> 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
> So please just let me know, I'm ready ;-)
Those changes seem good to me.
Quite happy to see this done with. Any issues can be addressed as bug fixes.