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
