[Asterisk-code-review] taskprocessor: Add high water mark warnings (asterisk[master])

[Mark Michelson]

Mark Michelson has posted comments on this change.

Change subject: taskprocessor: Add high water mark warnings
......................................................................


Patch Set 2:

(2 comments)

I'm not giving this a +1 or -1 because I basically just have observations more than findings.

https://gerrit.asterisk.org/#/c/1562/2/main/taskprocessor.c
File main/taskprocessor.c:

Line 719: #define HIGH_WATER_LEVEL 40
I'm curious what it takes in order to start seeing a backlog of 40 tasks get hit when Asterisk is running. Thinking about the uses of taskprocessors, I could see something like the SIP threadpool control taskprocessor or certain Stasis queues get this high during a heavy traffic storm but that isn't necessarily a dangerous indication. Maybe my instincts on this are wrong, but I think that the high-water mark should be much higher than this if we want to only start warning when we feel like the taskprocessor has backed up into "something has almost certainly gone wrong" territory. If tests have borne out that I'm incorrect on this, then that's fine and we can just leave it where it is.


Line 741: 	if (previous_size >= HIGH_WATER_LEVEL && !tps->high_water_warned) {
I suppose that Matt may be able to answer this better, but the current "warn once and only ever once" for a taskprocessor seems like it might not necessarily be the best plan. There are some taskprocessors (like serializers created for SIP dialogs) that are short-lived, but most taskprocessors stick around much longer. The result is that on a taskprocessor whose lifetime is approximately the same as the Asterisk process, we may see the high-water mark get hit at some point relatively early on, clear itself out for hours/days/weeks, and then during some new traffic explosion/deadlock condition/horrible catastrophe, hit the high-water mark again. The problem is that now on this second go-around, we don't see any sort of warning.

Limiting the warning certainly makes sense. You don't want to see the warning for every message beyond the mark, and you certainly don't want to see if a bunch if the taskprocessor keeps wavering between around the mark. But I feel like calling it one and done is too strict.


-- 
To view, visit https://gerrit.asterisk.org/1562
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib1607c35d18c1d6a0575b3f0e3ff5d932fd6600c
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Jonathan Rose <jrose at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Jonathan Rose <jrose at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes