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
>     updates.
>
> So please just let me know, I'm ready ;-)
>
> Thanks,
> Willy
Those changes seem good to me.
Merge it!
Quite happy to see this done with. Any issues can be addressed as bug fixes.

Thanks

-Patrick

Reply via email to